Unitful.jl icon indicating copy to clipboard operation
Unitful.jl copied to clipboard

Make the `isapprox` implementation closer to the LinearAlgebra implementation

Open eliascarv opened this issue 9 months ago • 0 comments

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

eliascarv avatar May 07 '24 18:05 eliascarv