Skip to content

fix: improve efficiency and accuracy of value_cast for quantity_point#765

Merged
mpusz merged 1 commit intomasterfrom
fix/efficient-value-cast-QP
Mar 7, 2026
Merged

fix: improve efficiency and accuracy of value_cast for quantity_point#765
mpusz merged 1 commit intomasterfrom
fix/efficient-value-cast-QP

Conversation

@mpusz
Copy link
Copy Markdown
Owner

@mpusz mpusz commented Mar 7, 2026

Overview

Rewrites the quantity_point overload of detail::sudo_cast to select the optimal intermediate unit and representation type for each conversion case, avoiding unnecessary operations and improving numerical accuracy.

This implements the design from #598 (originally authored by @burnpanck). The commit carries a Co-authored-by trailer to credit the original work. PR #598 has been closed in favour of this one.

Problem

The previous implementation always delegated cross-origin conversions through a recursive point_for() call (twice for the small-unit case), unconditionally rescaling to a common type before adding the origin offset. This forces extra arithmetic operations in cases where none are needed, and in the floating-point path could accumulate rounding error by expressing the offset in the wrong unit.

Additionally, there was a latent bug in the same-origin branch: the returned quantity_point was tagged with FromQP::point_origin instead of ToQP::point_origin.

Solution — four branches

Condition Strategy
Same origin Delegate entirely to the quantity sudo_cast. Also fixes the origin tag bug.
Same unit, different origin Skip unit scaling; compute the static origin offset in the common rep and add it directly. Always exact for integers.
Different unit, floating-point rep Pick the larger unit as the intermediate (minimises scaling magnitude), then call point_for() to let the library's common-unit arithmetic add the offset. This avoids precision loss that would occur if the offset were expressed explicitly in the larger unit (e.g. 42 m → 0.042 km loses bits).
Different unit, integer rep Prefer the output unit as intermediate (result is already in the right unit before truncation). Fall back to the input unit if the origin offset in the output unit would overflow the common rep type.

Tests

The existing value_cast tests for quantity_point are expanded to exercise every branch:

  • Branch 1 (same origin): existing tests retained.
  • Branch 2 (same unit, different origin): integer rep (both directions), and fractional double.
  • Branch 3 (FP, unit change): existing km→m test (from-unit larger); new m→km test (to-unit larger).
  • Branch 4 (integer, offset fits in output unit): existing int8_t mm → cm pair.
  • Branch 5 (integer, offset overflows output unit): new int16_t m → mm test where the 42 000 mm offset overflows int16_t, so the input unit (m) is used instead.

Related

Closes #598

Rewrite the quantity_point overload of sudo_cast to pick the best
intermediate unit/rep for each conversion case:

- Same origin: delegate entirely to the quantity sudo_cast (unchanged).
  Also fix a pre-existing bug where the result was incorrectly tagged
  with FromQP::point_origin instead of ToQP::point_origin.
- Same unit, different origin: skip unit scaling altogether; compute the
  origin offset in the common rep and add it directly.
- Different unit, floating-point rep: pick the larger of the two units
  as the intermediate (to minimise the scaling magnitude), then use
  point_for() to let the library's common-unit arithmetic handle the
  origin shift — this avoids the binary FP precision loss that occurs
  when the offset is expressed explicitly in the larger unit.
- Different unit, integer rep: prefer the output unit as intermediate so
  no extra scaling is needed after the offset addition. If the origin
  offset would overflow the output-unit type, fall back to the input
  unit where it is guaranteed to fit.

Also expand the value_cast tests for quantity_point to cover every
branch explicitly, including the new integer overflow-fallback path.

Co-authored-by: Yves Delley <burnpanck@users.noreply.github.com>
Closes: #598
@mpusz mpusz merged commit 10b4fcd into master Mar 7, 2026
168 checks passed
@mpusz mpusz deleted the fix/efficient-value-cast-QP branch March 7, 2026 21:53
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.

1 participant