hgrecco / pint

Operate and manipulate physical quantities in Python
http://pint.readthedocs.org/
Other
2.32k stars 456 forks source link

Unit equality can be tested as string with all units except dimensionless #634

Open lafrech opened 6 years ago

lafrech commented 6 years ago

I was just surprised by this.

I can't tell whether this is a minor bug or intended behaviour.

import pint
ureg = pint.UnitRegistry()

assert ureg.Unit('meter') == 'meter'
assert ureg.Unit('dimensionless') == 'dimensionless'
# AssertionError: assert <Unit('dimensionless')> == 'dimensionless'

This caught me while checking the unit of a quantity:

assert ureg.Quantity(12, 'meter').units == 'meter'
assert ureg.Quantity(12, 'dimensionless').units == 'dimensionless'
# AssertionError: assert <Unit('dimensionless')> == 'dimensionless'
hgrecco commented 6 years ago

We never thought about this. I usually compared to an empty string for dimensionless units or use the dimentionless property

>>> assert ureg.Unit('dimensionless') == ''
>>> ureg.Quantity(12, 'dimensionless').dimensionless
True

I am open to arguments in favor or opposing adding the case suggested in this issue.

cpascual commented 6 years ago

IMHO the current situation is inconsistent because for the other units the comparison works for the long string representation of the unit, which for dimensionless units, it would be "dimensionless", not "".

In fact, the abbreviated forms of other units do not compare well:

assert ureg.Unit('meter') == 'meter'
assert ureg.Unit('meter') == 'm'
# AssertionError

And just to open another front, neither do the alternate spellings:

assert ureg.Unit('metre') == 'meter'
assert ureg.Unit('metre') == 'metre'
# AssertionError

In conclusion, my preference would be to: 1) support the 'dimensionless' case for consistency 2) consider benefits vs overhead of a more generic implementation of the unit equality operator in order to support alternate spellings and abbreviated forms

hgrecco commented 6 years ago

I agree. I think that alternate spellings included in the definitions file will not be a problem but they will have an overhead. There are to options when comparing a Unit to string

  1. Get the string representation of the unit and compare to the string.
  2. Parse the string into a Unit

The first is fast but only handle very limited cases (individual units in canonical form, or compound units written in the same way that pint will write them). The second has an overhead (pint will need to parse the string) but will work in any situation. It is worth noting that for individual units or compound units already parsed the overhead will be only a dictionary lookup. So I think we should go for option number 2 by adding a specific case in Quantity.eq and Unit.eq for string comparisons.

Comments?

lafrech commented 6 years ago

I claim no expertise here, as I'm totally new to pint.

Maybe you could use 1 with a fallback to 2 in case of failure.

Compare string representation with string. If equal, return True. If not equal parse the string into a Unit and return comparison result.

This would add no overhead to the current working use case, if that's a concern. If the overhead is negligible, then option 2 alone may be cleaner code.

cpascual commented 6 years ago

I agree with going for option 2 (maybe with a first attempt with 1, as @lafrech suggests)

SimonHeybrock commented 1 year ago

Are there plans to implement this, or is there a decision on whether you would accept this as a contributed change? Having pint.Unit('m') == 'm' return False is confusing.