PainterQubits / Unitful.jl

Physical quantities with arbitrary units
Other
593 stars 111 forks source link

`atol` used by `isapprox` for affine units #723

Open sostock opened 2 months ago

sostock commented 2 months ago

As noted in https://github.com/PainterQubits/Unitful.jl/pull/719#discussion_r1600230695, the specified atol is stripped of its units by calling ustrip(unit(y), atol):

https://github.com/PainterQubits/Unitful.jl/blob/b80ab85b5e1bb59c376945e54e723a07b2330cc7/src/quantities.jl#L254

This means that the atol that is actually used depends on whether the units of x and y are affine or not:

julia> isapprox(274.15K, 275.15K, atol=10K) # passing everything in Kelvin gives the expected result
true

julia> isapprox(1°C, 2°C, atol=10K) # this should be the same …
false
sostock commented 2 months ago

The case where the atol itself is specified in °C might be more controversial:

julia> isapprox(1K, 2K, atol=0.1°C)
true

julia> isapprox(1°C, 2°C, atol=0.1°C)
false

IMO, these should both return true, since atol == 273.25K and 1K < 273.25K. But I can see that someone might disagree and think of isapprox(1°C, 2°C, atol=0.1°C) as equivalent to isapprox(1, 2, atol=0.1). Especially since logarithmic quantities already handle it like this:

julia> isapprox(20dBm, 21dBm, atol=10dBm) # equivalent to isapprox(20, 21, atol=10)
true

julia> isapprox(mW(20dBm), mW(21dBm), atol=mW(10dBm)) # equivalent to isapprox(100, 125.89…, atol=10)
false
cstjean commented 1 month ago

FWIW, over here we adhere strictly to "Temperature measurements can be either Kelvin or Celsius, temperature deltas must be in Kelvin", which Unitful nicely checks for us thanks to 0.1°C + 0.1°C being an AffineError. In that perspective, atol=0.1°C would be an error (too ambiguous to be safe).

(I'm aware this isn't necessarily a very popular perspective...)

EDIT: Ah, I guess it's https://github.com/PainterQubits/Unitful.jl/issues/521

sostock commented 1 month ago

I also think that making atol=0.1°C error is the best solution. And since the current behavior is broken (in my opinion), I don’t consider it a breaking change.

(Making all comparisons between affine and linear quantities error, as proposed in #521, is something I consider breaking, since the current behavior is well-defined and consistent, although unexpected to some and therefore error-prone).

cstjean commented 1 month ago

Fair enough!