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

Quantity comparison to dimensionless zero has an amusing edge case #15103

Closed
ayshih opened this issue Jul 31, 2023 · 14 comments
Closed

Quantity comparison to dimensionless zero has an amusing edge case #15103

ayshih opened this issue Jul 31, 2023 · 14 comments

Comments

@ayshih
Copy link
Contributor

ayshih commented Jul 31, 2023

Description

Comparing a quantity with units to dimensionless zero always works, presumably because for a given comparison it is reasonable to assume that dimensionless zero is the same as zero times a unit. However, this results in an amusing edge case when working with two units where the zero point does not coincide. For example, you can't normally compare degrees Celsius and Kelvin without the temperature() equivalency:

>>> -1*u.Celsius < 1*u.Kelvin
...
UnitConversionError: 'K' (temperature) and 'deg_C' (temperature) are not convertible
...
>>> -1*u.Celsius < (1*u.Kelvin).to(u.Celsius, equivalencies=u.temperature())
False

But, if you throw dimensionless zero into the mix, you seemingly can compare them without the equivalency (through transitivity) and then obtain the wrong answer.

>>> -1*u.Celsius < 0
True
>>> 1*u.Kelvin > 0
True
>>> -1*u.Celsius < 0 < 1*u.Kelvin
True

Expected behavior

Something not amusing, but this example is rather contrived

How to Reproduce

See description

Versions

Windows-10-10.0.22621-SP0
Python 3.10.10 | packaged by conda-forge | (main, Mar 24 2023, 20:00:38) [MSC v.1934 64 bit (AMD64)]
astropy 5.2.2
Numpy 1.24.3
pyerfa 2.0.0.3
Scipy 1.10.1
Matplotlib 3.7.1

@ayshih ayshih added the Bug label Jul 31, 2023
@Cadair Cadair added the units label Jul 31, 2023
@byrdie
Copy link
Contributor

byrdie commented Jul 31, 2023

This seems particularly hard to solve since the chained comparison operators are just syntactic sugar for (a < b) and (b < c), which would also have the same amusing behavior.

Seems like the only way out of this is to disable comparisons with dimensionless zero, but that would probably break a lot of downstream code.

Maybe the rich comparison chaining in PEP 535 would help?

@ayshih
Copy link
Contributor Author

ayshih commented Aug 4, 2023

Heh, the example in my original report was inspired by a live situation, but it's even more amusing since of course the units compared don't have to be related at all:

>>> 0*u.Celsius == 0*u.m
False
>>> 0*u.Celsius == 0 == 0*u.m
True

@pllim
Copy link
Member

pllim commented Aug 4, 2023

Maybe don't use == twice in the same statement? This is Python specific thing right? Not all languages even support such comparison.

@mhvk
Copy link
Contributor

mhvk commented Aug 6, 2023

I think the comparison with degrees C should just fail, i.e., any unit with an offset should not be allowed to compare with 0 without units.
In principle, this is not super-hard - deg_C and deg_F should become instances of a new function unit class (say OffsetUnit), which could disable this. But it needs time to implement...

@ayshih
Copy link
Contributor Author

ayshih commented Aug 6, 2023

Maybe don't use == twice in the same statement? This is Python specific thing right? Not all languages even support such comparison.

Well, as @byrdie noted, it's just syntactic sugar, until there is rich comparison chaining. As a stilted example, one could have a convenience function that implicitly leans on transitivity:

>>> def all_three_equal(a, b, c):
...     return (a == b) and (b == c)

And then argument order matters:

>>> all_three_equal(0*u.Celsius, 0, 0*u.m)
True
>>> all_three_equal(0*u.Celsius, 0*u.m, 0)
False

I think the comparison with degrees C should just fail, i.e., any unit with an offset should not be allowed to compare with 0 without units.

Sure, but that doesn't address the equality comparisons between unrelated units without an offset:

>>> 0*u.s == 0*u.m
False
>>> 0*u.s == 0 == 0*u.m
True

@pllim
Copy link
Member

pllim commented Aug 6, 2023

Do you think acknowledging this limitation in Known Issues would help?

@mhvk
Copy link
Contributor

mhvk commented Aug 6, 2023

Surely doesn't hurt. In the end, the reason for allowing 0 to have any dimension was simply that many things just worked a little easier (including some numpy functions such as np.sinc -- though these days one can just override).

@pllim
Copy link
Member

pllim commented Aug 10, 2023

Okay, I proposed text for known issue at #15154 . Do we keep this open even after that, or close this as "won't fix" ?

@byrdie
Copy link
Contributor

byrdie commented Aug 11, 2023

I personally think it would be best to deprecate comparison with dimensionless zero, but I'd be interested to hear what others think about it. I'm sure it would cause lots of problems downstream.

@mhvk
Copy link
Contributor

mhvk commented Aug 11, 2023

See #1254 for why this was added. Personally, I'm not sure I would decide the same way now, but it would definitely seem to be a case that one breaks a lot of code just for being slightly more "correct" -- which I don't think is worth it.

@pllim
Copy link
Member

pllim commented Aug 11, 2023

comparison with dimensionless zero

But I like that feature!

@mhvk
Copy link
Contributor

mhvk commented Aug 11, 2023

I think it is super-unlikely we can do much more than the warning @pllim added, so labelled this close?. I certainly have code that relies on being able to compare with 0 myself...

@github-actions
Copy link

Hi humans 👋 - this issue was labeled as Close? approximately 13 hours ago. If you think this issue should not be closed, a maintainer should remove the Close? label - otherwise, I will close this issue in 7 days.

If you believe I commented on this issue incorrectly, please report this here

@github-actions
Copy link

I'm going to close this issue as per my previous message, but if you feel that this issue should stay open, then feel free to re-open and remove the Close? label.

If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants