-
Notifications
You must be signed in to change notification settings - Fork 292
Rewrite every(), some(), and none() in C
#1169
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
Conversation
|
@DavisVaughan can you please take a look? It's been a while since I've touched C. |
Turns out that negate() adds too much overhead with C implementation of every()
fc99adc to
d54dfac
Compare
| if (is_na(out) && .allow_na) { | ||
| # Always return a logical NA | ||
| return(NA) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed this because no one else uses it
This was a fairly weird bit of purrr. every(), none(), and some() strictly required the result of .p to be TRUE or FALSE with no casting, but were lax about NA, allowing NA_character_ and friends, which I don't really think made any sense.
I've changed this in the C code to strictly require a scalar logical vector, and added tests about this.
This is a breaking change, but hopefully a very minor one.
| lifecycle::deprecate_soft( | ||
| when = "1.0.0", | ||
| what = I("Use of calls and pairlists in map functions"), | ||
| what = I("Use of calls and expressions in purrr functions"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe pairlists was a typo here
We now use vctrs_vec_compat() in every(), some(), and none() for consistency with map() and friends. That requires tweaking this message a little bit, but I think it is fine and unlikely to be seen by many people anyways.
| }) | ||
|
|
||
| test_that("pairlists, expressions, and calls are deprecated", { | ||
| local_options(lifecycle_verbosity = "warning") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make sure it warns every time it is called, not just the first time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of new tests to check all of the edge cases, our assumptions around what .p can return, and what we allow as input for .x.
|
Thanks @ErdaradunGaztea, we will take it from here! |
|
@DavisVaughan revdeps look good to me from a quick glance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG!
every() and related functionsevery(), some(), and none() in C
Fixes #1036
This PR reimplements
every(),some(), andnone()in C. I followed the C implementation ofmap()(since I have no experience writing in C). The interface does not change at all, the behaviour should also remain intact.The key change is I did not use
as_predicate(), butas_mapper()instead. Now, the difference between these two functions is that the former performs a Bool check on its output. This was computationally expensive and even replacing this check with.Call()didn't help, since the code was switching between C and R contexts a lot. My final solution was to perform these checks in C, in the same code that performs the predicate-checking loop.I had to replace the implementation of
none()with a separate C solution, sincenegate()had a huge overhead. However, all three functions share almost all of their C implementations now.Finally, the performance. They should be equal now (except that
every()and friends still have their early return).Created on 2025-02-06 with reprex v2.1.1