Skip to content

Round recovery-trace values in the double domain on Android - #1979

Merged
ryanbr merged 1 commit into
ryanbr:mainfrom
bhelm:fix/issue-47-recovery-trace-double-rounding
Sep 8, 2026
Merged

ryanbr merged 1 commit into
ryanbr:mainfrom
bhelm:fix/issue-47-recovery-trace-double-rounding

Conversation

@bhelm

@bhelm bhelm commented Sep 7, 2026

Copy link
Copy Markdown

Problem

The recovery trace rounds each rendered number to two decimals. Swift does
(x * 100).rounded(.toNearestOrAwayFromZero) / 100; Kotlin routed the same value through
Math.round, which returns a Long and therefore saturates. Any |x * 100| >= 2^63 came back
as Long.MAX_VALUE, so 1e20 rendered as 9.223372036854776e16 on Android where Apple kept
1e20 — the same trace line, two different numbers. The signed-zero half of bhelm#47 was
already fixed by the #1437 follow-up (copySign); this is the remaining large-finite half.

Change

  • android/app/src/main/java/com/noop/analytics/RecoveryScorerTrace.kt: r2 now rounds the
    magnitude in the Double domain — Math.rint plus an explicit half-away-from-zero step for an
    exact .5 tie — and reapplies the sign with copySign. Math.rint is half-to-even, so the
    tie step is required; floor(m + 0.5) is not usable for it, because m + 0.5 is itself rounded
    and would push 0.49999999999999994 up to 1.
  • Packages/StrandAnalytics/Sources/StrandAnalytics/RecoveryScorer+Trace.swift: the Swift math is
    unchanged, but the local r2 closure now forwards to a new named
    RecoveryScorer.traceRound2(_:) carrying the contract in its doc comment. Kotlin's r2 went
    private → internal for the same reason: without one named declaration per side, ±1e20 is
    untestable through the public trace API at all, since Swift renders it 1e+20 and Kotlin
    1.0E20 — the trace strings differ on formatting regardless of rounding. That is why the
    contract test compares raw IEEE bit patterns, not text.
  • Mirrored tests: RecoveryScorerTraceTests.swift (+57) and RecoveryScorerTraceTest.kt (+85),
    including a full 11-line trace fixture for one complete night with identical expected strings on
    both platforms.

Verification

  • Oracle: a standalone Swift twin (swiftc -O twin.swift -o t && ./t) produced every expected
    literal in the Kotlin test, used verbatim — e.g. -0.004 -> -0.0 bits=0x8000000000000000,
    1e20 -> 1e+20 bits=0x4415af1d78b58c40, 1e307 -> inf bits=0x7ff0000000000000. The twin source
    is reproduced in both test files' comments, per the existing repo convention.
  • Red before the change (with r2 made testable but its body unchanged):
    ./gradlew testFullDebugUnitTest --tests "com.noop.analytics.RecoveryScorerTraceTest" →
    10 tests completed, 1 failed — ComparisonFailure: r2(1e20) expected:<4[415af1d78b58c40]> but was:<4[3747ae147ae147b]> (= 9.223372036854776e16, the Long ceiling).
  • Green after: the same command with --rerun-tasks → BUILD SUCCESSFUL, XML tests="10" failures="0" errors="0", including traceRound2MatchesTheSwiftContract and
    traceFixtureIsByteIdenticalAcrossPlatforms.
    cd Packages/StrandAnalytics && swift test --filter RecoveryScorerTraceTests → Executed 10
    tests, with 0 failures
    (green before and after; the Swift math did not change).
  • python3 Tools/doc_comment_lint.py → OK no NEW detached doc comments, exit 0.
    Tools/i18n_audit.py not applicable: no user-facing strings.
  • Not run: macOS/iOS/watch app targets — no macOS/Xcode available to me. Neither changed file
    is compiled into an app-only target beyond StrandAnalytics, which is covered above. No hardware
    required.

Behaviour change / user-visible effects

Diagnostic trace text only. Charge, and every stored value, are unaffected. On Android a driver
term whose magnitude exceeds the Long ceiling now prints its real value instead of the saturated
9.2e16; in normal physiological ranges nothing changes. Documented in both doc comments: the
contract is defined for finite inputs, and non-finite inputs — or finite ones whose x * 100
overflows (|x| > ~1.79e306) — pass through as the matching infinity or NaN on both platforms. That
is pinned by the 1e306 / 1e307 rows rather than rejected, because Swift already behaved that
way and rejecting would have changed Swift.

Scope limits and follow-ups

  • Only RecoveryScorerTrace.r2 was changed. The identical Math.round-to-Long saturation
    pattern still exists at four other Android sites: HrvAnalyzerTrace.kt:15,
    SleepStagerTrace.kt:75, StepsEstimateEngineTrace.kt:29, and AnalyticsEngine.kt:1567,1585.
    Deliberately untouched here to keep one concern per PR — happy to follow up with a single PR that
    routes all five through one shared helper if you'd prefer that shape.
  • There is no pre-existing shared trace-fixture file to extend, so the mirrored fixture lives in the
    two twin test classes.

Branch: fix/issue-47-recovery-trace-double-rounding. Contribution offered under the repository's
PolyForm Noncommercial 1.0.0 license.

Math.round returns a Long, so the Kotlin trace helper saturated at 2^63-1:
1e20 rendered as 9.223372036854776e16 where the Swift twin kept 1e20, on a
diagnostic whose whole purpose is being comparable across platforms. Round the
magnitude with rint plus an explicit half-away-from-zero tie step instead, which
keeps every finite input (and the existing signed-zero handling) intact.

Names the shared contract as RecoveryScorer.traceRound2 on the Swift side and
pins it on both platforms by raw bit pattern (Swift and Kotlin print a Double
differently, so text would hide the divergence), plus an exact differential
fixture over one full-term night.

Charge scores are unchanged; only trace text is affected.

Refs: issue #47 (fork)
@ryanbr

ryanbr commented Sep 8, 2026

Copy link
Copy Markdown
Owner

Reviewed. Correct on both sides, and the Kotlin tie handling is right for a reason worth spelling out. Thank you @bhelm.

This is the same defect class you found in the strain map: Math.round returns a Long and therefore saturates, so a value Swift renders as 1e20 came back as 9.223372036854776e16. Two platforms, one input, two different diagnostics, which is the worst place for a divergence because the trace is what someone reads when they are already confused.

I worked through the replacement rather than trusting it:

val rounded = if (magnitude - floored == 0.5) floored + 1.0 else Math.rint(magnitude)
  • An exact half steps up by hand, giving half-away-from-zero once copySign restores the sign.
  • Everything else goes through rint, which is half-to-even, and that is safe only because the exact-tie case has already been peeled off. Worth keeping those two lines adjacent forever.
  • Non-finite inputs fall out correctly rather than by accident: floor(inf) - inf is NaN, NaN is not 0.5, and rint(inf) is inf. NaN behaves the same way. So infinities and NaN pass through as themselves, matching Swift.
  • Above 2^52 there is no fractional part, so the tie branch cannot fire and rint is the identity.

Your comment already explains why floor(m + 0.5) is not usable here, and it is the right call: m + 0.5 is itself rounded, so 0.49999999999999994 would be pushed up to 1. That is a real trap and I am glad it is written down next to the code rather than discovered again later.

Extracting traceRound2 on the Swift side is a good move too. The contract now has one named home per platform instead of a closure buried in a function, which is what makes "the Kotlin twin is RecoveryScorerTrace.r2" a checkable statement rather than a hope.

CI green, 16 of 16 against the head SHA. Swift I cannot compile here, so that side rests on CI. Nothing blocking.

@ryanbr
ryanbr merged commit c1e77b6 into ryanbr:main Sep 8, 2026
16 checks passed
@bhelm
bhelm deleted the fix/issue-47-recovery-trace-double-rounding branch September 15, 2026 22:35
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