search

yt-project / unyt

Handle, manipulate, and convert data with units in Python
https://unyt.readthedocs.io
BSD 3-Clause "New" or "Revised" License
360 stars 46 forks source link

Arithmetical operations between unit systems are not commutative for user defined units #148

Open Xarthisius opened 4 years ago

Xarthisius commented 4 years ago

Description

Order of operations (addition or multiplication) on quantities using two different unit systems for user defined units works only if user defined unit is used first.

As an example:

from unyt.unit_registry import *
from unyt.dimensions import dimensionless
from unyt.array import unyt_quantity

G = unyt_quantity(6.67384e-8, 'cm**3/g/s**2')

default_unit_registry = UnitRegistry(unit_system='cgs')
default_unit_registry.add('h', 1.0, dimensionless, tex_repr=r"h")

print(unyt_quantity(1, "Msun*h", default_unit_registry) * G)  # works
print(G * unyt_quantity(1, "Msun*h", default_unit_registry))  # fails

yields

1.32703693031024e+26 cm**3*h/s**2
Traceback (most recent call last):
  File "/home/xarth/codes/yt-project/unyt/unyt/unit_registry.py", line 72, in __getitem__
    ret = self.lut[str(key)]
KeyError: 'h'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/xarth/codes/yt-project/unyt/unyt/unit_registry.py", line 75, in __getitem__
    _lookup_unit_symbol(str(key), self.lut)
  File "/home/xarth/codes/yt-project/unyt/unyt/unit_registry.py", line 331, in _lookup_unit_symbol
    "Could not find unit symbol '%s' in the provided " "symbols." % symbol_str
unyt.exceptions.UnitParseError: Could not find unit symbol 'h' in the provided symbols.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "ala.py", line 11, in <module>
    print(G * unyt_quantity(1, "Msun*h", default_unit_registry))
  File "/home/xarth/codes/yt-project/unyt/unyt/array.py", line 1751, in __array_ufunc__
    mul, unit = unit_operator(u0, u1)
  File "/home/xarth/codes/yt-project/unyt/unyt/array.py", line 163, in _multiply_units
    ret = (unit1 * unit2).simplify()
  File "/home/xarth/codes/yt-project/unyt/unyt/unit_object.py", line 732, in simplify
    self.expr = _cancel_mul(expr, self.registry)
  File "/home/xarth/codes/yt-project/unyt/unyt/unit_object.py", line 767, in _cancel_mul
    u1 = _create_unit_from_factor(pair[0], registry)
  File "/home/xarth/codes/yt-project/unyt/unyt/unit_object.py", line 756, in _create_unit_from_factor
    f = registry[str(base)]
  File "/home/xarth/codes/yt-project/unyt/unyt/unit_registry.py", line 79, in __getitem__
    "The symbol '%s' does not exist in this registry." % key
unyt.exceptions.SymbolNotFoundError: The symbol 'h' does not exist in this registry.
ngoldbaum commented 4 years ago

Just a head’s up that I’m taking time off from code development, if you all need this fixed for yt then a PR implementing a fix would be probably the quickest way to get a fix merged.

ngoldbaum commented 4 years ago

Also from what I remember we did add h to yt’s default unit symbol table. In yt are you importing units from yt.units or from unyt directly? Keep in mind that yt.units and unyt have different units defined in their default namespaces. If you are getting this error despite inporting things from yt.units that seems incorrect to me, and might be a hint towars the source of the real issue.

Xarthisius commented 4 years ago

h is just one example of many similar failures. Here https://github.com/yt-project/yt/pull/2487 it was code_pressure. I'm pretty sure we're importing from yt.units, but why does it matter if we can reproduce the behavior outside of yt?

ngoldbaum commented 4 years ago

Well the example you used was with h...

Would the same issue not happen with yt.units on the current master branch, I would think that even before unyt both sides would need to have the dataset’s unit registry to know about code_pressure.

The reason I’m pushing back on this a bit is because it might be expensive to first check if the unit exists in the registry (a PR and benchmarks proving otherwise would change me mind though) and I think we were happy to work around that with yt.units. Could be wrong though, haven’t explicitly checked.

matthewturk commented 4 years ago

Hey, glad you're taking some time off. This suggestion has given me something to look into, so I'm trying to take a crack at it here before asking for anything else -- the issue we are actually seeing is the code_pressure symbol missing, and I'll either have a fix I issue a PR to yt or one that I issue here.

Enjoy your time away!