-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
vctrs revdep issues #212
Comments
Thank you for the advice and detailed notes! I appreciate you taking the time to test out the package. I will try to look more into this soon. |
Just as an FYI we will send vctrs to CRAN soon (this week or next week), which will cause your tests to fail. You will get a two week notice from CRAN at some point requiring a fix in your package. |
Thanks for the heads up! I implemented enough of your suggested changes to fix the failing tests. I probably have more to do before a CRAN release ideally, but I will be able to get out a release in time either way. |
Hi there, we are planning on releasing a new version of vctrs soon, and this package was flagged in our revdeps. You should be able to reproduce this yourself by installing the dev version of vctrs and running your tests. You should get some failures related to taxa-authority.
In particular, the taxa-authority class seems to be a rcrd class where a "non missing" observation can still have
NA
fields. i.e.This often causes issues with how vctrs methods work. For example:
vec_detect_complete()
won't see these as "complete" observations because they look partially missingvec_compare()
won't be able to compare two authority vectors right, because theNA
s propagateThe revdep actually flagged an issue with
xtfrm()
, which is used byorder()
. Thextfrm.vctrs_vctr()
method is now powered byvctrs:::vec_rank(x, ties = "dense", incomplete = "na")
. Because these observations don't look "complete", they automatically get anNA
ranking, which is used inorder()
.I think I can offer a few pieces of advice to make taxa more compatible with vctrs:
Maybe use
""
for the citation field when the user didn't supply one? That way we/you can differentiate that from an actualNA
.rcrd classes don't officially support names yet, but I see you are trying to use them anyways. I think it is ok for the
.names
field to be in the result ofvec_proxy()
, because you want it to be sliced along with the data, but it is not ok for.names
to be present in the result ofvec_proxy_equal()
, because names shouldn't be considered in equality comparisons. Moreover,.names
is oftenNA
and this will cause the problems mentioned above. Adding avec_proxy_equal()
method that removes the.names
field will also handlevec_proxy_compare()
andvec_proxy_order()
(since they callvec_proxy_equal()
by default).I think if you fix those last two bullets, then you'll have a more pleasant experience using vctrs with taxa, and it should fix most of the issues mentioned above
The text was updated successfully, but these errors were encountered: