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

atol used by isapprox for affine units #723

Open
sostock opened this issue May 17, 2024 · 4 comments
Open

atol used by isapprox for affine units #723

sostock opened this issue May 17, 2024 · 4 comments
Labels
affine affine quantities like °C, °F, …

Comments

@sostock
Copy link
Collaborator

sostock commented May 17, 2024

As noted in #719 (comment), the specified atol is stripped of its units by calling ustrip(unit(y), atol):

return isapprox(x.val, y.val; atol=ustrip(unit(y), atol), kwargs...)

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 sostock added the affine affine quantities like °C, °F, … label May 17, 2024
@sostock
Copy link
Collaborator Author

sostock commented May 17, 2024

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
Copy link
Contributor

cstjean commented May 20, 2024

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 #521

@sostock
Copy link
Collaborator Author

sostock commented May 20, 2024

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
Copy link
Contributor

cstjean commented May 21, 2024

Fair enough!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affine affine quantities like °C, °F, …
Projects
None yet
Development

No branches or pull requests

2 participants