Rewrite reduce() and family in C#1246
Open
ErdaradunGaztea wants to merge 30 commits intotidyverse:mainfrom
Open
Rewrite reduce() and family in C#1246ErdaradunGaztea wants to merge 30 commits intotidyverse:mainfrom
reduce() and family in C#1246ErdaradunGaztea wants to merge 30 commits intotidyverse:mainfrom
Conversation
Currently only 1 arg, requires .init, doesn't support directions or accumulation
This would make it more difficult to reason about `SEXP bar` in particular, as we'd not know it is protected already, and that's a very bad pattern.
Contributor
Author
|
Note that my last commit was necessitated by the latest vctrs (I had v0.6.5 installed) release adding extra information to the errors. If you'd rather snatch these updated snaps out of this PR because they do not fit the theme here - you're welcome. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1168 and closes #1243. Your acceptation of #1169 prompted me to tackle a bigger task 🦔
This PR reimplements
reduce()(andreduce2(),accumulate(), andaccumulate2()as well) in C. The idea came from Hadley's comment on #1168 stating that it'd be hard to performantly add progress bars toreduce()as they are written in C, so I rewrote the function in C instead. It is more performant too, gettingreduce()to be on par with baseReduce():Created on 2026-01-18 with reprex v2.1.1
accumulate()isn't quite as fast as in base R, but there's improvement nonetheless:Created on 2026-01-28 with reprex v2.1.1
I tried not to change any behavior (except for #1243) and hopefully succeeded, though that meant adding a lot of tests in the end. That was also the reason I didn't tackle #1245, as it'd change non-erroring behavior.
As for the code, well... It's not pretty, but I couldn't think of any approach that would look cleaner with interacting
.init,.dir, anddone(), and trying to make one shared implementation for all four functions with only some differences in certain points of the algorithm. If you prefer, I can separate C loops forreduce()andaccumulate()family, that would probably be the most influential one. If I were to say, it's probably 15-20% more complex than the current R implementation (numbers completely made-up).Finally, the C implementation should make it very easy to add
.dirparameter toreduce2()andaccumulate2(), as well as makeaccumulate2()return a named vector if input is named, likeaccumulate()does. Then again, I didn't want to add more changes than necessary and I can always add it later if you so wish.No worries, I'm quickly running out of purrr functions to optimize :)