Skip to content
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

More stringent range testing #48465

Merged
merged 4 commits into from
Feb 1, 2023
Merged

More stringent range testing #48465

merged 4 commits into from
Feb 1, 2023

Conversation

timholy
Copy link
Member

@timholy timholy commented Jan 31, 2023

Turns a @test_skip into a @test. This may have been passing since #23194 but was never turned into a requirement. This change removes the single largest contribution to "total broken tests" in the whole test suite.

Turns a test_skip into a test. This may have been passing since
PR#23194 but never made a requirement. This change removes the
single largest contribution to "total broken tests" in the whole
test suite.
@LilithHafner
Copy link
Member

Error in testset ranges:
--
  | Test Failed at C:\buildkite-agent\builds\win2k22-amdci6-4\julialang\julia-master\julia-3d18b7b681\share\julia\test\ranges.jl:890
  | Expression: stop ≈ last(r)
  | Evaluated: -6.67572f-5 ≈ -6.681681f-5

@timholy
Copy link
Member Author

timholy commented Jan 31, 2023

Yeah, there seem to be a few edge cases left. Would could allow a small failure rate, or just close this. Thoughts?

@simonbyrne
Copy link
Contributor

Why is it failing only on some platforms? Is it not deterministic?

@simonbyrne
Copy link
Contributor

simonbyrne commented Jan 31, 2023

An example case I found:

julia> (start, delta, stop)  = -2.8345144f0, 0.9447948f0, -0.00012993813f0
(-2.8345144f0, 0.9447948f0, -0.00012993813f0)

julia> r = start:delta:stop
-2.8345144f0:0.9447948f0:-0.00013005733f0

julia> last(r)  stop
false

The problem here is that

julia> start + 3*delta  # intermediate values in Float32
-0.00012993813f0

julia> Float32(Float64(start) + 3*Float64(delta)) # intermediate values in higher precision
-0.00013005733f0

This particular case uses the fallback branch, since the rational part fails:

julia> num, den = Base.rat(delta)
(599, 634)

julia> Float32(num/den)
0.94479495f0

The question here is what criteria we should use for determining delta and stop?

The fix for now is to adjust the tolerance.

test/ranges.jl Outdated Show resolved Hide resolved
@simonbyrne simonbyrne closed this Jan 31, 2023
@simonbyrne simonbyrne reopened this Jan 31, 2023
@simonbyrne simonbyrne added test This change adds or pertains to unit tests ranges Everything AbstractRange labels Jan 31, 2023
test/ranges.jl Outdated Show resolved Hide resolved
Co-authored-by: Ian Butterworth <[email protected]>
@timholy
Copy link
Member Author

timholy commented Jan 31, 2023

LGTM, thanks @simonbyrne

@simonbyrne simonbyrne merged commit e498fef into master Feb 1, 2023
@simonbyrne simonbyrne deleted the teh/range_skip branch February 1, 2023 01:29
@vtjnash
Copy link
Member

vtjnash commented Feb 6, 2023

Seen on CI:

The global RNG seed was 0xf5fc6174bf915507070615bf2c22eaf4.
Error in testset ranges:
Test Failed at /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-macmini-aarch64-4.0/build/default-macmini-aarch64-4-0/julialang/julia-master/julia-d7b2062924/share/julia/test/ranges.jl:890
  Expression: ≈(stop, last(r), atol = eps((n - 1) * Δ) + eps(stop))
   Evaluated: -1.6152623f0 ≈ -1.6152625f0 (atol=1.7881393e-7)

simonbyrne added a commit that referenced this pull request Feb 6, 2023
Follow up on #48465.

Addresses CI failure noted here #48465 (comment).
simonbyrne added a commit that referenced this pull request Feb 9, 2023
* Fix bounds on floating point range testing

Follow up on #48465.

Addresses CI failure noted here #48465 (comment).

* Use simpler error bound
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ranges Everything AbstractRange test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants