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

Add tests provided by Aqua.jl #687

Merged
merged 17 commits into from
Dec 9, 2024
Merged

Add tests provided by Aqua.jl #687

merged 17 commits into from
Dec 9, 2024

Conversation

lbenet
Copy link
Member

@lbenet lbenet commented Oct 12, 2024

No description provided.

@lbenet
Copy link
Member Author

lbenet commented Oct 12, 2024

I propose to add Aqua.jl to check some ambiguities, etc, in IntervalArithmetic. In doing so, I noticed the following ambiguities (using Julia v1.10.5):

method_ambiguity = (_interval_infsup(::Type{T}, x::Union{BareInterval, Interval}, y, d::IntervalArithmetic.Decoration) where T<:Union{AbstractFloat, Rational} @ IntervalArithmetic ~/.julia/dev/IntervalArithmetic/src/intervals/construction.jl:472, _interval_infsup(::Type{T}, a, b::Complex, d::IntervalArithmetic.Decoration) where T<:Union{AbstractFloat, Rational} @ IntervalArithmetic ~/.julia/dev/IntervalArithmetic/src/intervals/construction.jl:499)
method_ambiguity = (_interval_infsup(::Type{T}, x, y::Union{BareInterval, Interval}, d::IntervalArithmetic.Decoration) where T<:Union{AbstractFloat, Rational} @ IntervalArithmetic ~/.julia/dev/IntervalArithmetic/src/intervals/construction.jl:483, _interval_infsup(::Type{T}, a::Complex, b, d::IntervalArithmetic.Decoration) where T<:Union{AbstractFloat, Rational} @ IntervalArithmetic ~/.julia/dev/IntervalArithmetic/src/intervals/construction.jl:497)
method_ambiguity = (interval(::Type{T}, a; ...) where T @ IntervalArithmetic ~/.julia/dev/IntervalArithmetic/src/intervals/construction.jl:416, interval(a, d::IntervalArithmetic.Decoration; format) @ IntervalArithmetic ~/.julia/dev/IntervalArithmetic/src/intervals/construction.jl:420)
method_ambiguity = (union!(::AbstractVector{S}, ::Interval, ::Any, ...) where S @ IntervalArithmetic ~/.julia/dev/IntervalArithmetic/src/intervals/real_interface.jl:120, union!(::AbstractVector{S}, ::Any, ::BareInterval, ...) where S @ IntervalArithmetic ~/.julia/dev/IntervalArithmetic/src/intervals/real_interface.jl:124)
method_ambiguity = (kwcall(::NamedTuple, ::typeof(interval), ::Type{T}, a) where T @ IntervalArithmetic ~/.julia/dev/IntervalArithmetic/src/intervals/construction.jl:416, kwcall(::NamedTuple, ::typeof(interval), a, d::IntervalArithmetic.Decoration) @ IntervalArithmetic ~/.julia/dev/IntervalArithmetic/src/intervals/construction.jl:420)
method_ambiguity = (ExactReal(value::T) where T<:Real @ IntervalArithmetic ~/.julia/dev/IntervalArithmetic/src/intervals/exact_literals.jl:54, (::Type{T})(x::ExactReal) where T<:Real @ IntervalArithmetic ~/.julia/dev/IntervalArithmetic/src/intervals/exact_literals.jl:109)
method_ambiguity = (union!(::AbstractVector{S}, ::BareInterval, ::Any, ...) where S @ IntervalArithmetic ~/.julia/dev/IntervalArithmetic/src/intervals/real_interface.jl:120, union!(::AbstractVector{S}, ::Any, ::Interval, ...) where S @ IntervalArithmetic ~/.julia/dev/IntervalArithmetic/src/intervals/real_interface.jl:124)

I plan to fix them in this PR...

@OlivierHnt
Copy link
Member

Nice! It does not seem so bad?

The 6th item on the list is easy to fix, we just need to add

ExactReal(x::ExactReal) = x

Though I do not know how we can solve item 4 and 7.

@OlivierHnt
Copy link
Member

To resolve the first two items about _interval_infsup we can add

_interval_infsup(::Type{T}, a::Complex, b::Union{BareInterval,Interval}, d::Decoration = com) where {T<:NumTypes} =
    complex(_interval_infsup(T, real(a), real(b), d), _interval_infsup(T, imag(a), imag(b), d))
_interval_infsup(::Type{T}, a::Union{BareInterval,Interval}, b::Complex, d::Decoration = com) where {T<:NumTypes} =
    complex(_interval_infsup(T, real(a), real(b), d), _interval_infsup(T, imag(a), imag(b), d))

@OlivierHnt
Copy link
Member

Items 3 and 5 should be fixed by throwing an explicit error, e.g.

interval(T::Type, d::Decoration; format::Symbol = :infsup) = throw(MethodError(interval, (T, d)))

@lbenet
Copy link
Member Author

lbenet commented Oct 12, 2024

Thanks a lot for the suggestions @OlivierHnt, they nicely solve most ambiguities.

(My solution 47605eb did solve the ambiguities, but triggered other errors, so I'm reverting it.)

@lbenet
Copy link
Member Author

lbenet commented Oct 12, 2024

I think we solved all pure-IntervalArithmetic ambiguities. Yet, there seem to be other, seemingly related to ForwardDiff; see here and the next lines:

method_ambiguity = (promote_rule(::Type{ForwardDiff.Dual{T, V, N}}, ::Type{R}) where {T, V, N, R<:Real} @ ForwardDiff ~/.julia/packages/ForwardDiff/PcZ48/src/dual.jl:428, promote_rule(::Type{T}, ::Type{Interval{S}}) where {T<:Real, S<:Union{AbstractFloat, Rational}} @ IntervalArithmetic ~/work/IntervalArithmetic.jl/IntervalArithmetic.jl/src/intervals/construction.jl:553)
method_ambiguity = (promote_rule(::Type{R}, ::Type{ForwardDiff.Dual{T, V, N}}) where {R<:Real, T, V, N} @ ForwardDiff ~/.julia/packages/ForwardDiff/PcZ48/src/dual.jl:427, promote_rule(::Type{ExactReal{T}}, ::Type{S}) where {T<:Real, S<:Real} @ IntervalArithmetic ~/work/IntervalArithmetic.jl/IntervalArithmetic.jl/src/intervals/exact_literals.jl:117)
method_ambiguity = ((ForwardDiff.Dual{T, V})(x) where {T, V} @ ForwardDiff ~/.julia/packages/ForwardDiff/PcZ48/src/dual.jl:79, (::Type{T})(x::ExactReal) where T<:Real @ IntervalArithmetic ~/work/IntervalArithmetic.jl/IntervalArithmetic.jl/src/intervals/exact_literals.jl:111)
method_ambiguity = (promote_rule(::Type{ForwardDiff.Dual{T, V, N}}, ::Type{R}) where {T, V, N, R<:Real} @ ForwardDiff ~/.julia/packages/ForwardDiff/PcZ48/src/dual.jl:428, promote_rule(::Type{T}, ::Type{ExactReal{S}}) where {T<:Real, S<:Real} @ IntervalArithmetic ~/work/IntervalArithmetic.jl/IntervalArithmetic.jl/src/intervals/exact_literals.jl:115)
method_ambiguity = (==(x::Real, y::ForwardDiff.Dual{Ty}) where Ty @ ForwardDiff ~/.julia/packages/ForwardDiff/PcZ48/src/dual.jl:145, ==(x::Union{BareInterval, Interval}, y::Number) @ IntervalArithmetic ~/work/IntervalArithmetic.jl/IntervalArithmetic.jl/src/intervals/real_interface.jl:155)
method_ambiguity = (convert(::Type{ForwardDiff.Dual{T, V, N}}, x) where {T, V, N} @ ForwardDiff ~/.julia/packages/ForwardDiff/PcZ48/src/dual.jl:435, convert(::Type{T}, x::ExactReal) where T<:Real @ IntervalArithmetic ~/work/IntervalArithmetic.jl/IntervalArithmetic.jl/src/intervals/exact_literals.jl:113)
method_ambiguity = (convert(::Type{ForwardDiff.Dual{T, V, N}}, x::Number) where {T, V, N} @ ForwardDiff ~/.julia/packages/ForwardDiff/PcZ48/src/dual.jl:436, convert(::Type{T}, x::ExactReal) where T<:Real @ IntervalArithmetic ~/work/IntervalArithmetic.jl/IntervalArithmetic.jl/src/intervals/exact_literals.jl:113)
method_ambiguity = (ForwardDiff.Dual(args...) @ ForwardDiff ~/.julia/packages/ForwardDiff/PcZ48/src/dual.jl:73, (::Type{T})(x::ExactReal) where T<:Real @ IntervalArithmetic ~/work/IntervalArithmetic.jl/IntervalArithmetic.jl/src/intervals/exact_literals.jl:111)
method_ambiguity = (==(x::ForwardDiff.Dual{Tx}, y::Real) where Tx @ ForwardDiff ~/.julia/packages/ForwardDiff/PcZ48/src/dual.jl:144, ==(x::Number, y::Union{BareInterval, Interval}) @ IntervalArithmetic ~/work/IntervalArithmetic.jl/IntervalArithmetic.jl/src/intervals/real_interface.jl:156)
method_ambiguity = ((ForwardDiff.Dual{T})(value) where T @ ForwardDiff ~/.julia/packages/ForwardDiff/PcZ48/src/dual.jl:69, (::Type{T})(x::ExactReal) where T<:Real @ IntervalArithmetic ~/work/IntervalArithmetic.jl/IntervalArithmetic.jl/src/intervals/exact_literals.jl:111)
method_ambiguity = (ForwardDiff.Dual{T, V, N}(x) where {T, V, N} @ ForwardDiff ~/.julia/packages/ForwardDiff/PcZ48/src/dual.jl:77, (::Type{T})(x::ExactReal) where T<:Real @ IntervalArithmetic ~/work/IntervalArithmetic.jl/IntervalArithmetic.jl/src/intervals/exact_literals.jl:111)
method_ambiguity = (promote_rule(::Type{R}, ::Type{ForwardDiff.Dual{T, V, N}}) where {R<:Real, T, V, N} @ ForwardDiff ~/.julia/packages/ForwardDiff/PcZ48/src/dual.jl:427, promote_rule(::Type{Interval{T}}, ::Type{S}) where {T<:Union{AbstractFloat, Rational}, S<:Real} @ IntervalArithmetic ~/work/IntervalArithmetic.jl/IntervalArithmetic.jl/src/intervals/construction.jl:550)
method_ambiguity = (ForwardDiff.Dual{T, V, N}(x::Number) where {T, V, N} @ ForwardDiff ~/.julia/packages/ForwardDiff/PcZ48/src/dual.jl:78, (::Type{T})(x::ExactReal) where T<:Real @ IntervalArithmetic ~/work/IntervalArithmetic.jl/IntervalArithmetic.jl/src/intervals/exact_literals.jl:111)

@OlivierHnt
Copy link
Member

Why do the tests only error for Julia 1.9?

Yes they are all related to ForwardDiff.jl. Maybe it's because we did not extend the functions properly in IntervalArithmeticForwardDiffExt?

Project.toml Outdated Show resolved Hide resolved
@lbenet
Copy link
Member Author

lbenet commented Oct 13, 2024

Why do the tests only error for Julia 1.9?

Tests fail in Julia v1.9 because we @test that the number of ambiguities is zero (and not yet for other versions!) 😄.

This has to be changed, certainly, but since the new LTS Julia version is v1.10.5, I think we should also get rid of most conditionals involving VERSION and require the LTS version at least. There are actually not too many cases involving VERSION, but all except one involve comparissons related to v1.10.

Yes they are all related to ForwardDiff.jl. Maybe it's because we did not extend the functions properly in IntervalArithmeticForwardDiffExt?

I'll try to have a look on them later, and perhaps add tests for those that yield ambiguities that I cannot solve.

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 78.13%. Comparing base (7ff253f) to head (3ca341d).
Report is 17 commits behind head on master.

Files with missing lines Patch % Lines
src/intervals/construction.jl 0.00% 3 Missing ⚠️
src/intervals/real_interface.jl 0.00% 2 Missing ⚠️
src/intervals/exact_literals.jl 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #687      +/-   ##
==========================================
- Coverage   81.02%   78.13%   -2.89%     
==========================================
  Files          27       28       +1     
  Lines        2324     2639     +315     
==========================================
+ Hits         1883     2062     +179     
- Misses        441      577     +136     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines 7 to 8
Base.promote_rule(::Type{Dual{T, V, N}}, ::Type{Interval{S}}) where {T, V, N, S<:Union{AbstractFloat, Rational}} =
Interval{IntervalArithmetic.promote_numtype(V, S)}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this should return Interval or Dual{T, Interval{IntervalArithmetic.promote_numtype(V, S), N}...?

@OlivierHnt Any ideas?

Same for ExactReal...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I am note familiar with ForwardDiff.jl... My first impulse would be Dual{T, Interval{IntervalArithmetic.promote_numtype(V, S), N}.
Maybe @Kolaru knows about this more? (though I believe he is very busy at the moment with the PhD defence)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Locally, tests pass (though I do not get the messages on the ambiguities) using Dual{T, Interval{IntervalArithmetic.promote_numtype(V, S), N} too. I thought about this after pushing the commit, and checking the methods defined for Dual's...

@OlivierHnt
Copy link
Member

After rebasing this PR, the number of method ambiguities is much larger due to the fast matrix product #682.
I have to look at it more closely. But my opinion is that if these cannot be easily solved with a clever dispatch mechanism, we should just add @test_broken, and merge this PR, focusing on only native IA functions + ForwardDiff.Dual.

@lbenet
Copy link
Member Author

lbenet commented Nov 13, 2024

Thanks @OlivierHnt for rebasing and taking a look. I'll try to work on this today (there are issues to access our campus today...)

@OlivierHnt OlivierHnt merged commit da867a2 into master Dec 9, 2024
32 checks passed
@OlivierHnt OlivierHnt deleted the lb/aqua branch December 9, 2024 17:20
@lbenet
Copy link
Member Author

lbenet commented Dec 9, 2024

Thanks a lot @Kolaru for continuing with this... I've been too busy with other (less interesting, more administrative) stuff.

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.

4 participants