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

Default isapprox() definition differs from Base julia #710

Open
staticfloat opened this issue Feb 29, 2024 · 3 comments
Open

Default isapprox() definition differs from Base julia #710

staticfloat opened this issue Feb 29, 2024 · 3 comments

Comments

@staticfloat
Copy link
Contributor

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

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.

@giordano
Copy link
Collaborator

giordano commented Mar 4, 2024

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?".

@sostock
Copy link
Collaborator

sostock commented Mar 12, 2024

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.

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

No branches or pull requests

3 participants