Rework test suite - #775
Open
OlivierHnt wants to merge 21 commits into
Open
Conversation
… cut Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
@Kolaru can you look closely at the changes in piecewise.jl? It looked fine to me, but since I do not use this feature I cannot tell for sure. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Benchmark Results (Julia v1)Time benchmarks
Memory benchmarks
|
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Remove the platform-dependent Float16 CoreMath pin, relax the BLAS thread-count assertions to round-trip invariants, and remove the Makie and RecipesBase tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rational literals are Rational{Int32} on 32-bit systems.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…Diff tag comparison CoreMath ships no library for 32-bit systems. The unqualified import of the tag ordering conflicts with the one of IntervalArithmetic.Symbols on Julia 1.10. 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 #775 +/- ##
===========================================
+ Coverage 75.56% 94.05% +18.48%
===========================================
Files 32 33 +1
Lines 3131 3179 +48
===========================================
+ Hits 2366 2990 +624
+ Misses 765 189 -576 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…gebra Needed for Julia v1.10. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The display now bypasses the digit rounding whenever the shortest representation of a bound fits within the significant digits, matching the output of Julia Base independently of the architecture. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Kolaru
requested changes
Aug 26, 2026
Kolaru
left a comment
Kolaru
left a comment
Member
There was a problem hiding this comment.
I didn't look into all tests in details (still I commented on what I caught), but I had a look at all the changes in the source files I believe.
| lo = ifelse(lo == typemin(T), _value_min(T), lo) | ||
| hi = ifelse(hi == typemax(T), _value_max(T), hi) | ||
| val = convert(T, (1 - β) * lo + β * hi) | ||
| val = _value_round(T, (1 - β) * lo + β * hi) |
Member
There was a problem hiding this comment.
I am not sure that this is the correct fix.
T is only the bound type. In principle, it says nothing about the type of the numbers in between, it is fine to return a Float64 for Rational bounds.
Furthermore, I don't know if rationalize keeps the distribution uniform.
If we would follow the convention that Distributions.jl uses for Uniform, then we would always return Float64, regardless of the bound type:
julia> using Distributions
julia> u = Uniform(0//1, 1//1)
Uniform{Rational{Int64}}(a=0//1, b=1//1)
julia> typeof(rand(u))
Float64
julia> u = Uniform(0.0f0, 1f0)
Uniform{Float32}(a=0.0f0, b=1.0f0)
julia> typeof(rand(u))
Float64
Member
Author
There was a problem hiding this comment.
Ok let's output a float for either AbstractFloat or Rational bound type.
Comment on lines
89
to
92
| function (constant::Constant)(x::Interval) | ||
| y = interval(constant.value) | ||
| return _unsafe_interval(bareinterval(y), decoration(y), isguaranteed(x)) | ||
| end |
Member
There was a problem hiding this comment.
I don't think that this is the right handling of the guarantee here:
It doesn't matter if the input interval is correct or not, the output will always be the same. If there is a doubt about the bound of the interval and whether a piece of the function should be used or not, it should go to PieceWise instead.
On the other hand, we are currently trusting the value inside the Constant... we could argue that this should return a NG interval, except for Constant{ExactReal}. I'm not sure what's best here.
Member
Author
There was a problem hiding this comment.
Ok let's revert this.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The reduction operations do not pertain to interval arithmetic. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rework intersect_domain via rightof and leftof on bounds, revert the guarantee handling of Constant, and test a single point out of the domain. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
Ok so there are two things worth discussing a bit more:
|
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
|
I'm fine with not having sample for Rational intervals. Regarding I believe that the main point of So I would actually be in favor of keeping the current convention that the constant is considered to be exact. I would however make it clear in the docstring and wrap the constant in As a side note, it would be nice if |
Member
|
Or actually, now that I am thinking about it, we could deprecate |
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
3 participants
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.
Complete rework of the test suite, and fixes for the bugs it surfaced in
src/.Test suite
test/now mirrors the organization ofsrc/andext/. Every extension is covered (DiffRules, IntervalSets, Makie, RecipesBase and SparseArrays previously had no tests). The legacytest/interval_tests/is removed, and every assertion was preserved or strengthened.dot_nearest@test_broken. Theatan2override is also removed, since the original ITL expectationdacis correct and now passes.Fixes in
src/atan: the decoration of a box touching the branch cut from above is capped atdac, sincecomrequires continuity at each point of the box.acot: uses the conventionacot(0) = π/2, with the jump at 0 decorated like the branch cut of the two-argumentatan. An explicit method forIntervalreplaces the Base fallbackatan(inv(x)).:ulprounding mode: functions without a CoreMath routine (cot,sec,csc,acot,coth,sech,csch,acoth) fall back to the:correctimplementation.Piecewise:ArgumentErrors on mismatched numbers of domains, functions or junctions (wasMethodErrororUndefVarError).in_domainis rewritten. It wrongly returnedfalsefor inputs starting at a closed endpoint or lying in a later piece.isguaranteedpropagation:PiecewiseandConstantno longer return guaranteed intervals from non-guaranteed inputs.intersect_domain: at equal endpoint values, an open bound now wins ([0,1] ∩ [0,1)is[0,1)).Domainbound validation throwsArgumentError(wasErrorException). Dead helpers removed and docstrings corrected.🤖 Generated with Claude Code