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

Make the isapprox implementation closer to the LinearAlgebra implementation #719

Merged
merged 6 commits into from
May 17, 2024

Conversation

eliascarv
Copy link
Contributor

@eliascarv eliascarv commented May 7, 2024

The Unitful implementation of isapprox(::AbstractArray, ::AbstractArray) function has some differences compared to the LinearAlgebra implementation.
Link: https://github.com/JuliaLang/julia/blob/master/stdlib/LinearAlgebra/src/generic.jl#L1879-L1891

For example, the comparison in Unitful is:

d <= atol + rtol*max(norm(x), norm(y))

But in LinearAlgebra is:

iszero(rtol) ? d <= atol : d <= max(atol, rtol*max(norm(x), norm(y)))

Another difference is in the use of the Base.rtoldefault function.
The Base.rtoldefault(x, y, atol) function returns the default rtol only when atol is less than or equal to zero, otherwise it returns zero.
But, in Unitful, the last argument of Base.rtoldefault is always zero:

rtol::Real=Base.rtoldefault(T1,T2,0),
atol=zero(Quantity{real(T1),D,U1})

That is, the rtol value is always be different than zero, even when atol > 0.
Below is the LinearAlgebra implementation for comparison:

atol::Real=0,
rtol::Real=Base.rtoldefault(promote_leaf_eltypes(x),promote_leaf_eltypes(y),atol)

The last difference is that the Unitful doesn't forward the nans keyword argument.

These different implementations result in different behaviours:
master branch:

julia> x = [-48.044483f0, -18.32653f0]
2-element Vector{Float32}:
 -48.044483
 -18.32653

julia> y = [-48.04448f0, -18.326504f0]
2-element Vector{Float32}:
 -48.04448
 -18.326504

julia> isapprox(x, y, atol=1f-5)
false

julia> isapprox(x * u"m", y * u"m", atol=1f-5u"m")
true

This PR adjusts the implementation to be closer to the LinearAlgebra implementation:

julia> x = [-48.044483f0, -18.32653f0]
2-element Vector{Float32}:
 -48.044483
 -18.32653

julia> y = [-48.04448f0, -18.326504f0]
2-element Vector{Float32}:
 -48.04448
 -18.326504

julia> isapprox(x, y, atol=1f-5)
false

julia> isapprox(x * u"m", y * u"m", atol=1f-5u"m")
false

src/quantities.jl Show resolved Hide resolved
src/quantities.jl Outdated Show resolved Hide resolved
@eliascarv eliascarv requested a review from sostock May 10, 2024 23:10
src/quantities.jl Outdated Show resolved Hide resolved
Co-authored-by: Sebastian Stock <[email protected]>
@eliascarv eliascarv requested a review from sostock May 14, 2024 20:16
@eliascarv
Copy link
Contributor Author

eliascarv commented May 15, 2024

@sostock, PR ready for final review and merge.
And, after the merge, can you release a new version please?

Copy link
Collaborator

@sostock sostock left a comment

Choose a reason for hiding this comment

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

There should be some tests for the new behavior. Suggestions:

@test isapprox([1.0*m, NaN*m], [nextfloat(1.0)*m, NaN*m], nans=true)
@test !isapprox([1.0*m, NaN*m], [nextfloat(1.0)*m, NaN*m], nans=false)
@test !isapprox([1.0m, 2.0m], [1.1m, 2.2m], rtol=0.05, atol=0.2m)
@test !isapprox([1.0m], [nextfloat(1.0)*m], atol = eps(0.1)*m)

If I didn’t make a mistake, all of these tests should error/fail on the old version and pass with this PR.

The scalar isapprox(::AbstractQuantity{T,D,U}, ::AbstractQuantity{T,D,U}) where {T,D,U} method should be changed to use ustrip(absoluteunit(unit(y)), atol), (and tests should be added for that as well), but that is unrelated to the changes in this PR and can be done later.

After this PR is merged, I will make a new release (a minor release, since in my opinion the addition of the nans keyword qualifies as a feature).

src/quantities.jl Outdated Show resolved Hide resolved
@eliascarv eliascarv requested a review from sostock May 16, 2024 11:38
Co-authored-by: Sebastian Stock <[email protected]>
@eliascarv
Copy link
Contributor Author

@sostock ready for merge

@sostock sostock merged commit efc3802 into PainterQubits:master May 17, 2024
13 checks passed
@eliascarv eliascarv deleted the isapprox branch May 17, 2024 11:08
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.

3 participants