r-lib / styler

Non-invasive pretty printing of R code
https://styler.r-lib.org
Other
725 stars 71 forks source link

Prefer vctrs functions over slower base R equivalents #1114

Closed krlmlr closed 1 year ago

krlmlr commented 1 year ago

for performance.

github-actions[bot] commented 1 year ago

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 940010d41129746d44e52d44664a37e44559bd39 is merged into main:

Further explanation regarding interpretation and methodology can be found in the documentation.

krlmlr commented 1 year ago

Next in line: replacing x[idx, ] with vec_slice(x, idx) .

krlmlr commented 1 year ago

And split() by vec_split() . Both operations are use a lot, the vctrs version is faster by a factor.

github-actions[bot] commented 1 year ago

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 0c4187c1bd6acec209292edd94b34496f42b3225 is merged into main:

Further explanation regarding interpretation and methodology can be found in the documentation.

lorenzwalthert commented 1 year ago

I still see the definition of bind_rows() and some uses in the source code. Is that intentional or did you want to replace all occurrences?

Screenshot 2023-04-17 at 17 08 04
krlmlr commented 1 year ago

Gone now.

krlmlr commented 1 year ago

Will conflict with #1112, but I can handle that.

codecov-commenter commented 1 year ago

Codecov Report

Merging #1114 (9efa6d5) into main (0592e8e) will decrease coverage by 0.01%. The diff coverage is 90.66%.

:exclamation: Current head 9efa6d5 differs from pull request most recent head e4435d6. Consider uploading reports for the commit e4435d6 to get more accurate results

@@            Coverage Diff             @@
##             main    #1114      +/-   ##
==========================================
- Coverage   91.07%   91.07%   -0.01%     
==========================================
  Files          46       46              
  Lines        2713     2701      -12     
==========================================
- Hits         2471     2460      -11     
+ Misses        242      241       -1     
Impacted Files Coverage Δ
R/relevel.R 49.29% <28.57%> (ø)
R/utils-navigate-nest.R 81.48% <50.00%> (ø)
R/compat-dplyr.R 92.59% <66.66%> (-0.27%) :arrow_down:
R/compat-tidyr.R 100.00% <100.00%> (ø)
R/detect-alignment-utils.R 97.18% <100.00%> (ø)
R/detect-alignment.R 97.84% <100.00%> (+0.04%) :arrow_up:
R/indent.R 100.00% <100.00%> (ø)
R/nest.R 100.00% <100.00%> (ø)
R/parse.R 87.80% <100.00%> (ø)
R/reindent.R 100.00% <100.00%> (ø)
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

github-actions[bot] commented 1 year ago

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 0592e8e3e214da15d115145a9e00a554ddbb2064 is merged into main:

Further explanation regarding interpretation and methodology can be found in the documentation.

github-actions[bot] commented 1 year ago

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 0592e8e3e214da15d115145a9e00a554ddbb2064 is merged into main:

Further explanation regarding interpretation and methodology can be found in the documentation.

github-actions[bot] commented 1 year ago

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 0592e8e3e214da15d115145a9e00a554ddbb2064 is merged into main:

Further explanation regarding interpretation and methodology can be found in the documentation.

github-actions[bot] commented 1 year ago

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 0592e8e3e214da15d115145a9e00a554ddbb2064 is merged into main:

Further explanation regarding interpretation and methodology can be found in the documentation.

github-actions[bot] commented 1 year ago

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 0592e8e3e214da15d115145a9e00a554ddbb2064 is merged into main:

Further explanation regarding interpretation and methodology can be found in the documentation.

github-actions[bot] commented 1 year ago

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 0592e8e3e214da15d115145a9e00a554ddbb2064 is merged into main:

Further explanation regarding interpretation and methodology can be found in the documentation.

krlmlr commented 1 year ago

Finally green.

Force-pushed, with meaningful individual commits.

github-actions[bot] commented 1 year ago

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 0592e8e3e214da15d115145a9e00a554ddbb2064 is merged into main:

Further explanation regarding interpretation and methodology can be found in the documentation.

krlmlr commented 1 year ago

Super-green. Greener than green.

lorenzwalthert commented 1 year ago

Great @krlmlr. I was wondering: Do we want to replace all / more of [ with vec_slice()? Because I see more cases, some of them (like in lag()) get called on every nest, so there might still be room for improvement.

krlmlr commented 1 year ago

Good catch! [ on a vector is fast, but we do gain a lot with data frames. Expecting another boost with the last commit.

github-actions[bot] commented 1 year ago

This is how benchmark results would change (along with a 95% confidence interval in relative change) if e5ecef721dd414873566526ab9ae432ecb3de4d5 is merged into main:

Further explanation regarding interpretation and methodology can be found in the documentation.