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

Default `isapprox()` definition differs from Base julia

Open staticfloat opened this issue 1 year ago • 3 comments

The isapprox() definition in this package does not match that of Julia's, causing differences when units are applied versus when they are not:

x = 1.0u"s"
y = 1.0u"s" + 1e-4u"s"

@show isapprox(x, y; rtol=1e-4)
@show isapprox(ustrip(x), y; rtol=1e-4)
@show isapprox(x, ustrip(y); rtol=1e-4)
@show isapprox(ustrip(x), ustrip(y); rtol=1e-4)

Yields:

isapprox(x, y; rtol = 0.0001) = true
isapprox(ustrip(x), y; rtol = 0.0001) = false
isapprox(x, ustrip(y); rtol = 0.0001) = false
isapprox(ustrip(x), ustrip(y); rtol = 0.0001) = true

staticfloat avatar Feb 29 '24 01:02 staticfloat

It appears the issue is that it just directly fails if one of the values is missing its units. I'm not sure if that is intended behavior or not.

staticfloat avatar Mar 04 '24 20:03 staticfloat

I'd say it's intentional, it doesn't make sense to compare a unitless quantity to a unitful one, they have different physical dimensions, it's like the apples to oranges comparison. I wonder if this should rather error to be loud and clear, although I can see why "false" is an appropriate answer to the question "are they approximately equal?".

giordano avatar Mar 04 '24 21:03 giordano

I think it would be good to make the case of different dimensions error instead of returning false. The other kind of dimension mismatch (array dimension instead of physical dimension) already errors:

julia> isapprox([1,2,3], [1,2])
ERROR: DimensionMismatch: dimensions must match: a has dims (Base.OneTo(3),), b has dims (Base.OneTo(2),), mismatch at 1

However, this should probably be considered a breaking change, since the current behavior seems intentional.

sostock avatar Mar 12 '24 20:03 sostock