vctrs revdep issues

[DavisVaughan]

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.

library(taxa)

x <- taxon_authority(
  c('Cham. & Schldl.', 'L.', 'Billy'), 
  date = c('1827', '1753', '2000')
)

x
#> <taxon_authority[3]>
#> [1] Cham. & Schldl. 1827 L. 1753              Billy 2000

# Notice how there are NAs in some of the fields, but neither are considered "missing" observations
vec_proxy_equal(x)
#>   .names          author date citation
#> 1   <NA> Cham. & Schldl. 1827     <NA>
#> 2   <NA>              L. 1753     <NA>
#> 3   <NA>           Billy 2000     <NA>

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 missing
• vec_compare() won't be able to compare two authority vectors right, because the NAs propagate

The revdep actually flagged an issue with xtfrm(), which is used by order(). The xtfrm.vctrs_vctr() method is now powered by vctrs:::vec_rank(x, ties = "dense", incomplete = "na"). Because these observations don't look "complete", they automatically get an NA ranking, which is used in order().

library(taxa)

x <- taxon_authority(
  c('Cham. & Schldl.', 'L.', 'Billy'), 
  date = c('1827', '1753', '2000')
)

# They don't look like "complete" observations because the
# .names and citation fields of the equality proxy are NA
vec_detect_complete(x)
#> [1] FALSE FALSE FALSE

# Because they don't look "complete", our new xtfrm() method
# ranks them with NA, because anything else is ambiguous
xtfrm(x)
#> [1] NA NA NA
vctrs:::vec_rank(x, ties = "dense", incomplete = "na")
#> [1] NA NA NA

# Same problem comes up here,
# NAs propagate so `x` doesn't look equivalent with itself
vec_compare(x, x)
#> [1] NA NA NA

I think I can offer a few pieces of advice to make taxa more compatible with vctrs:

• For your proxies, it is best if the values in a row are either all missing or all non-missing. For example here is what clock does:

library(clock)

# notice how the NA propagated to all fields
vctrs::vec_proxy(year_month_day(c(2019, 2019), c(12, NA)))
#>   year month
#> 1 2019    12
#> 2   NA    NA

• Maybe use "" for the citation field when the user didn't supply one? That way we/you can differentiate that from an actual NA.

• 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 of vec_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 of vec_proxy_equal(), because names shouldn't be considered in equality comparisons. Moreover, .names is often NA and this will cause the problems mentioned above. Adding a vec_proxy_equal() method that removes the .names field will also handle vec_proxy_compare() and vec_proxy_order() (since they call vec_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

[zachary-foster]

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.

[DavisVaughan]

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.

[zachary-foster]

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.