-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test floating-point arrays using isapprox #318
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #318 +/- ##
==========================================
+ Coverage 95.25% 99.88% +4.63%
==========================================
Files 8 8
Lines 906 908 +2
==========================================
+ Hits 863 907 +44
+ Misses 43 1 -42 ☔ View full report in Codecov by Sentry. |
0ca1978
to
923c046
Compare
Should this be merged? |
I don't understand this statement. All basic floating point operations are exactly defined and should never change. |
Sorry, I wasn't clear. There are two factors here, and it's not about floating-point arithmetic directly. The first issue is that Julia may return different random numbers for the same seed on different versions: julia> VERSION
v"1.9.4"
julia> rng = MersenneTwister(123456); randn(rng, Float64)
-0.620399355121865 vs julia> VERSION
v"1.11.0-DEV.1071"
julia> rng = MersenneTwister(123456); randn(rng, Float64)
-1.6434956816309902 The second issue is that indexing into a range and an array may lead to different results, as the former involves a different set of floating-point operations than the latter. So, on nightly, julia> A
5-element Fill{Float64}, with entries equal to -1.6434956816309902
julia> B
6:10
julia> (A + B)[end]
8.356504318369009
julia> A[end] + B[end]
8.35650431836901
julia> (A + B)[end] == A[end] + B[end]
false
julia> (A + B)[end] ≈ A[end] + B[end]
true On the other hand, on v1.9.4 julia> rng = MersenneTwister(123456); A = Fill(randn(rng, Float64), 5)
5-element Fill{Float64}, with entries equal to -0.620399355121865
julia> B = 6:10
6:10
julia> A[end] + B[end]
9.379600644878135
julia> (A + B)[end]
9.379600644878135
julia> (A + B)[end] == A[end] + B[end]
true Taken together, a test comparing two values obtained through different sets of operations and starting from different points may happen to pass coincidentally on one version of Julia, but fail on a different version. We should therefore compare these approximately and not exactly. We may use |
Gentle bump |
Gentle bump |
Bump. It would be good to have a decision on this, as this should get tests passing on unreleased versions of julia |
@@ -380,22 +380,22 @@ end | |||
# type, and produce numerically correct results. | |||
as_array(x::AbstractArray) = Array(x) | |||
as_array(x::UniformScaling) = x | |||
equal_or_undef(a::Number, b::Number) = (a == b) || isequal(a, b) | |||
equal_or_undef(a, b) = all(equal_or_undef.(a, b)) | |||
isapprox_or_undef(a::Number, b::Number) = (a ≈ b) || isequal(a, b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is fine.
I had though might want to flip order because of types that mightt define isapprox
but would define isequal
.
But if you restrict to Number
subtypes there is a fallback defintion for isapprox
that does call ==
at least.
And I think that has you covered
The equality need not hold, as floating-point results may change by a few ULP across Julia versions. Using
isapprox
is the more reasonable choice. This gets tests working on the current Julia nightly.