Fix checked_mul off-by-one, non-finite rem platform dependence, and relax Statistics compat - #330
Merged
Merged
Conversation
The divisor 2^f-1 is odd, so a widened product equal to typemax(N).i * rawone(N) + (rawone(N) >> 1) still rounds to typemax(N) and must not throw. Previously such products (e.g. checked_mul(1.228N1f7, 1.638N1f7)) spuriously raised OverflowError, disagreeing with both wrapping_mul and the generic float fallback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
_rem skipped the isfinite guard when bitwidth(T) < 32, leaving the result to unspecified float-to-int conversion behavior: Inf % N0f8 returned 1.0N0f8 on aarch64 but 0.0N0f8 on x86_64. Check isfinite unconditionally so NaN/Inf map to zero on all platforms, matching the wider types. This also makes saturating_fdiv(0, 0) well-defined. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The pin below v1.11.2 was awaiting the next Statistics.jl release (JuliaStats/Statistics.jl#165). Statistics v1.11.4 is now registered, still provides _mean_promote, and the extension already guards its use with isdefined; the full test suite passes against v1.11.4. Without this, environments requiring newer Statistics downgrade FixedPointNumbers to 0.8. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The workaround from JuliaMath#287 rewrote the literal line 'version = "0.9.0-dev"' to 0.8.4 so that the invalidations action can co-install SnoopCompile (whose dependency FlameGraphs pins FixedPointNumbers to <= 0.8). The literal no longer matches now that the version is 0.9.x, so the resolver hit unsatisfiable requirements. Match any version line instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #330 +/- ##
=======================================
Coverage 96.42% 96.42%
=======================================
Files 7 7
Lines 784 784
=======================================
Hits 756 756
Misses 28 28 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Description:
Three small fixes for v0.9.1, found while reviewing the v0.8.5...v0.9.0 diff.
Fix off-by-one in the overflow check of
checked_mulforNormedThe specialized integer path rejects a widened product
zwhenz >= mwherem = typemax(N).i * rawone(N) + (rawone(N) >> 1). Butrawone(N) = 2^f - 1is odd, soz == mstill rounds to exactlytypemax(N)and should not throw:This disagreed with both wrapping_mul (returns typemax, no wrap) and the generic float fallback (z < typemax(N) + eps(N)/2, which accepts values that round to typemax). Changed the comparison to z <= m and added regression tests for boundary cases in N1f7 and N7f9. Verified exhaustively for all Normed{UInt8} types that checked_mul now throws exactly when the rounded result exceeds typemax, and agrees with wrapping_mul otherwise.
Make rem of non-finite values platform-independent for small types
_rem(x::Real, ...) skipped the isfinite guard when bitwidth(T) < 32, leaving the result to unspecified float-to-int conversion behavior (the "reduced likelihood" residue of #291): Inf % N0f8 returned 1.0N0f8 on aarch64 but 0.0N0f8 on x86_64. The guard is now unconditional for both Fixed and Normed, so NaN/±Inf map to zero on all platforms, matching the 32- and 64-bit types. This also makes saturating_fdiv(x, zero(x)) well-defined. test_rem_nan now covers ±Inf and its "TODO: avoid undefined behavior" is resolved.
Relax Statistics compat to allow v1.11.2 and later
Project.toml said "Update this version specifier when Statistics.jl v1.11.2 is released" (JuliaStats/Statistics.jl#165) — Statistics v1.11.4 is now registered, so the "< 1.11.2" pin has become a trap: any environment requiring newer Statistics silently downgrades FixedPointNumbers to 0.8.6. Statistics v1.11.4 still provides _mean_promote, the extension already guards its use with isdefined, and the full test suite passes against v1.11.4. Relaxed to Statistics = "1".
Fix version spoof in the Invalidations workflow
The invalidations CI job was failing on this PR (and would fail on any PR since the 0.9.0 version bump) for reasons unrelated to the code: the workaround from #287 rewrites the version in Project.toml to 0.8.4 so the resolver can co-install SnoopCompile, whose dependency FlameGraphs pins
FixedPointNumbers = "0.6.1-0.8". That rewrite matched the literal lineversion = "0.9.0-dev", which no longer exists, so the real 0.9.x version reached the resolver and produced "Unsatisfiable requirements ... FlameGraphs". The spoof now matches anyversion =line. Only the version label is spoofed — the invalidation measurement still loads this branch's source. BothOverwrite Package Version # FIXMEsteps can be deleted entirely once FlameGraphs publishes a release whose compat allows FixedPointNumbers 0.9.