Added isapprox support for MultiValue

[miguelmaso]

This PR is a proposal to extend the interface of MultiValue. Its implementation follows the pattern of #1157.

Current behaviour
Right now, the function isapprox(::MultiValue, ::MultiValue) is available with default arguments, but is not possible to pass optional arguments like tolerance.

Proposed behaviour
After this PR, it will be able to call the function isapprox with optional arguments, e.g. isapprox(A,B,rtol=eps(1.0).

[Antoinemarteau]

Hello Miguel, thanks for your PR :)

I gave some comments, I think this can be merged after taking them into account and updating README.md.

[miguelmaso]

I gave some comments, I think this can be merged after taking them into account and updating README.md.

Added to NEWS.md

[miguelmaso]

Hi @Antoinemarteau , thanks for your review. I've applied your suggestions. However, you'll see that I've faced limitations when applying some of the suggestions. The PR is ready to review again.

[JordiManyer]

I am just not sure why we are not just adding the kwargs to the pre-existing signature, instead of having two different ones?

Like why not just change this:

(≈)(a::MultiValue{S},b::MultiValue{S}) where {S} = isapprox(get_array(a), get_array(b))
(≈)(a::MultiValue{S,T1,N,0} where T1,b::MultiValue{S,T2,N,0} where T2) where {S,N} = true

to this:

(≈)(a::MultiValue{S},b::MultiValue{S}; kwargs...) where {S} = isapprox(get_array(a), get_array(b); kwargs...)
(≈)(a::MultiValue{S,T1,N,0} where T1,b::MultiValue{S,T2,N,0} where T2; kwargs...) where {S,N} = true

or am I missing something?

In my mind there is no need to have one signature with kwargs and one without.

[Antoinemarteau]

I agree @JordiManyer, I suggested replacing the line cause I liked removing {S} from the signature because isapprox([1 2], [1 2 3]) throws a DimensionMismatch error.
If we keep {S}, we get a method error for missmatching ::MultiValue sizes, otherwise the next dispatch falls back to the mismatch error.

So I would keep

(≈)(a::MultiValue,b::MultiValue; kwargs...) = isapprox(get_array(a), get_array(b); kwargs...)
(≈)(a::MultiValue{S,T1,N,0} where T1,b::MultiValue{S,T2,N,0} where T2; kwargs...) where {S,N} = true

[JordiManyer]

Sure, I think we can even remove the zero-dimensional dispatch and just keep the first signature, right? I don't think it gets used much and even if it does I presume the fallback is just as fast?

[miguelmaso]

I am just not sure why we are not just adding the kwargs to the pre-existing signature, instead of having two different ones?

Like why not just change this:

(≈)(a::MultiValue{S},b::MultiValue{S}) where {S} = isapprox(get_array(a), get_array(b))
(≈)(a::MultiValue{S,T1,N,0} where T1,b::MultiValue{S,T2,N,0} where T2) where {S,N} = true

to this:

(≈)(a::MultiValue{S},b::MultiValue{S}; kwargs...) where {S} = isapprox(get_array(a), get_array(b); kwargs...)
(≈)(a::MultiValue{S,T1,N,0} where T1,b::MultiValue{S,T2,N,0} where T2; kwargs...) where {S,N} = true

or am I missing something?

In my mind there is no need to have one signature with kwargs and one without.

I'm afraid the binary operator ≈ takes only two arguments. From the documentation:

The binary operator ≈ is equivalent to isapprox with the default arguments

[JordiManyer]

Then lets keep

isapprox(a::MultiValue,b::MultiValue;kwargs...) = isapprox(get_array(a),get_array(b);kwargs...)

and remove the rest.

[miguelmaso]

Then lets keep

isapprox(a::MultiValue,b::MultiValue;kwargs...) = isapprox(get_array(a),get_array(b);kwargs...)

and remove the rest.

Got it. Done

[Antoinemarteau]

I'm afraid the binary operator ≈ takes only two arguments. From the documentation:

Its a minor detail, but this doesn't mean we need to overload isapprox. The "binary operator" refers to the infix notation a ≈ b. But ≈ is also an alias to isapprox.

So it is not needed to import the isapprox symbol, importing and defining methods on ≈ is enough.
(The only reason why a isapprox b doesn't work is because of the parsing).

I also agree that we can remove the signature for empty tensors, the new default works the same.

[miguelmaso]

I'm afraid the binary operator ≈ takes only two arguments. From the documentation:

Its a minor detail, but this doesn't mean we need to overload isapprox. The "binary operator" refers to the infix notation a ≈ b. But ≈ is also an alias to isapprox. So it is not needed to import the isapprox symbol, importing and defining methods on ≈ is enough. (The only reason why a isapprox b doesn't work is because of the parsing).

Thanks for the explanation. Done

[miguelmaso]

I also agree that we can remove the signature for empty tensors, the new default works the same.

Removed line 8

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.27%. Comparing base (b4d3cdc) to head (10cfda6).
⚠️ Report is 11 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1158      +/-   ##
==========================================
- Coverage   89.27%   89.27%   -0.01%     
==========================================
  Files         211      211              
  Lines       26693    26692       -1     
==========================================
- Hits        23830    23829       -1     
  Misses       2863     2863              

Flag	Coverage Δ
drivers	0.00% <0.00%> (ø)
tests	89.27% <100.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[miguelmaso]

Thank you very much for your attention!