Skip to content

Convert once per distinct pair of units instead of once per value - #445

Merged
Enchufa2 merged 4 commits into
r-quantities:mainfrom
billdenney:perf-vectorize-conversions
Sep 28, 2026
Merged

Enchufa2 merged 4 commits into
r-quantities:mainfrom
billdenney:perf-vectorize-conversions

Conversation

@billdenney

Copy link
Copy Markdown
Contributor

Summary

ud_convert() and ud_are_convertible() called into udunits once per element through mapply(). Converting a units vector of n values therefore made n native calls, and each one parsed both units and built a converter. That includes every units<- conversion, which passes a single pair of units. Mixed units had the same per-element cost:

  • mixed_units() called set_units() once per element, parsing the unit string for every value.
  • set_units() on mixed units grouped its values with keys from as.character(), which pretty-prints every element's units.

Changes

  • ud_convert() and ud_are_convertible() convert the whole vector in one call when there is a single pair of units. Otherwise they convert once per distinct pair and write the results back in order.
  • Arguments whose lengths mapply() would warn about or refuse, and non-atomic x, still go through mapply(). Results, names, warnings and errors are therefore unchanged.
  • mixed_units() builds one units vector per distinct unit and splits it into one units object per value.
  • set_units() on mixed units groups with ud_char() keys.
  • A NEWS entry.

Behaviour is unchanged

All results are identical() to element-by-element conversion.

  • 169 new test expectations compare against the per-element results over an enumerated grid of inputs and 50 seeded random mixes of units.
  • They pin order, names (NA-padded when recycled), NA/NaN/Inf, zero-length input, the recycling warning, list x, error cases, and mixed-units construction and conversion.
  • The full suite passes: 753 expectations, 0 failures, 0 warnings.
  • A deliberately broken version, which writes results back reversed within each group, fails 39 of the equivalence tests, the order tests and three existing tests.

Speed

At 10^6 values:

Operation Before After
set_units(), single unit pair 11.3 s 2 ms
x + y 9.6 s 3 ms
ud_convert(), mixed units 8.7 s 134 ms
mixed_units() 4.1 min 6.8 s
set_units() on mixed units 5.6 min 22 s

Every size from 100 values up is faster.

Relationship to other open PRs

Trial merges with #443 and with the long-unit-format fix conflict only in NEWS.md and tests/testthat/test_udunits.R, where each branch appends entries. Keep both sides. R/udunits.R merges cleanly with #443.

🤖 Generated with Claude Code

billdenney and others added 4 commits September 27, 2026 20:57
ud_convert() and ud_are_convertible() called into udunits once per element
through mapply(), so converting a units vector of n values made n native
calls, each parsing both units and building a converter. That includes
every units<- conversion, which passes a single pair of units.

Convert the whole vector in one call when there is a single pair of units,
and otherwise once per distinct pair, writing the results back in order.
Arguments whose lengths mapply() would warn about or refuse, and non-atomic
x, still go through mapply(), so results, names, warnings and errors are
unchanged.

Tests compare against element by element conversion over a grid of inputs
and 50 random mixes of units, and pin order, names, NA, zero lengths and
recycling.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mixed_units() called set_units() once per element, parsing the unit string
for every value, and set_units() on mixed units rebuilt its result the same
way after grouping its values with keys from as.character(), which
pretty-prints every element's units.

Build one units vector per distinct unit and split it into one units object
per value, and group with ud_char() keys instead. Results are identical to
building the elements one by one, which the tests check.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Enchufa2
Enchufa2 merged commit a823fef into r-quantities:main Sep 28, 2026
8 checks passed
@codecov

codecov Bot commented Sep 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.92%. Comparing base (7180a90) to head (1b3f7cd).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #445      +/-   ##
==========================================
+ Coverage   92.56%   92.92%   +0.36%     
==========================================
  Files          20       20              
  Lines        1156     1201      +45     
==========================================
+ Hits         1070     1116      +46     
+ Misses         86       85       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@billdenney
billdenney deleted the perf-vectorize-conversions branch September 28, 2026 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants