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

checking for unit compatibility #988

Closed
keewis opened this issue Jan 15, 2020 · 7 comments · Fixed by #993
Closed

checking for unit compatibility #988

keewis opened this issue Jan 15, 2020 · 7 comments · Fixed by #993

Comments

@keewis
Copy link
Contributor

keewis commented Jan 15, 2020

What is the official way to check compatibility between units (or arrays and units)? Right now I'm using

array.check(unit)  # for mixed array / unit
array.dimensionality == unit.dimensionality  # same as above

array.dimensionality == other_array.dimensionality  # for two arrays
unit.dimensionality == other_unit.dimensionality  # for two units

but it took me some time to realize this was possible (it is not really obvious to me and I couldn't find it in the docs -- I might have missed it, though).

I usually put this in a function named something like is_compatible to make it a little bit more robust. If there is no such function yet, is there any interest in adding one?

@hgrecco
Copy link
Owner

hgrecco commented Jan 15, 2020

Dimensionality was the intended way to compare this. Let's define:

>>> u1 = ureg.meter ** 3
>>> u2 = ureg.liter
>>> q1 = 10 * u1
>>> q2 = 4 * u2

Using dimensionality works for quantities and units:

>>> q1.dimensionality == q2.dimensionality # quantity to quantity
True
>>> q1.dimensionality == u2.dimensionality # quantity to units
True
>>> u1.dimensionality == u2.dimensionality # units to units
True

I have also seen other things in the wild

>>> (q1/q2).dimensionless. # quantity to quantity
True
>>> (q1/u2).dimensionless. # quantity to units
True
>>> (u1/u2).dimensionless. # units to units
True

but this is not a good idea as it fails when one of the units is non-multiplicative.

check came afterwards (taking the name from check and wraps). Using check works for quantities:

>>> q1.check(u2) # Quantity to unit
True
>>> q1.check(q2) # Quantity to quantity
True

but it does not work for units to units.

Finally, we created get_compatible_units (and defined compatible for equal dimensionality).

In general, I would be in favour of adding a method (e.g. is_compatible) to Quantity and Unit as long as is contexts aware. I would also be in favour of deprecating Quantity.check. But I would like some more opinions.

@keewis
Copy link
Contributor Author

keewis commented Jan 15, 2020

I was thinking about a top-level function like

def is_compatible(obj1, obj2, unit_registry=pint._DEFAULT_REGISTRY):
    def get_dimensionality(obj):
	if isinstance(obj, (unit_registry.Quantity, unit_registry.Unit)):
            unit_like = obj
	else:
            unit_like = unit_registry.dimensionless

	return unit_like.dimensionality

    return get_dimensionality(obj1) == get_dimensionality(obj2)

but I have no idea how to make it context-aware or how to deal with units from different unit registries

@hgrecco
Copy link
Owner

hgrecco commented Jan 15, 2020

I would also agree with a top level function (I still think that a method for Quantity and Unit would be nice also).

PS.- I think you have a bug in your code, unit_registry.dimensionless will return a bool`.

Regearding making it unit aware, the easiest way is to add the folowing arguments to your function
*contexts, **ctx_kwargs and then try to convert the quantity and check for errors:

try:
    quantity.to(new_unit, *contexts, **ctx_kwargs)
    return True
except pint.errors.DimensionalityError:
    return False

It woud be nice to have a context aware get_dimensionality but is not possible because there will be multiple results.

@keewis
Copy link
Contributor Author

keewis commented Jan 15, 2020

the bug happens when not using the default registry: then the isinstance check will always return False so the unit gets lost. I think using hasattr(obj, "dimensionality") instead could help.

Regarding your proposed implementation: that could work, but only when quantities are involved (just like Quantity.check)

@hgrecco
Copy link
Owner

hgrecco commented Jan 15, 2020

I think the implementation could go along the following lines:

In Quantity

def is_compatible(self, other, *contexts, **ctx_kwargs):
    if contexts:
        try:
            self.to(other, *contexts, **ctx_kwargs)
            return True
        except pint.errors.DimensionalityError:
            return False
    if isinstance(other, (Quantity, Unit)): 
        return self.dimensionality == other.dimensionality 
    if isinstance(other, str):
        return self.dimensionality == self._REGISTRY.parse_units(other).dimensionality
    return self.dimensionless

In Unit:

def is_compatible(self, other, *contexts, **ctx_kwargs):
    if contexts:
        try:
            (1 * quantity).to(other, *contexts, **ctx_kwargs)
            return True
        except pint.errors.DimensionalityError:
            return False
    if isinstance(other, (Quantity, Unit)): 
        return self.dimensionality == other.dimensionality 
    if isinstance(other, str):
        return self.dimensionality == self._REGISTRY.parse_units(other).dimensionality
    return self.dimensionless

Top level function:

def is_compatible(obj1, obj2, unit_registry=None, *contexts, **ctx_kwargs):
    ureg = unit_registry or pint._DEFAULT_REGISTRY # or should be the APP registry????
    if isintance(obj1, (ureg.Quantity, ureg.Unit)):
        return obj1.is_compatible(obj2, *contexts, **ctx_kwargs)
    if isinstance(obj1, str):
        return unit_registry.parse_expression(obj1).is_compatible(obj2, *contexts, **ctx_kwargs)
    return not isinstance(obj2, (ureg.Quantity, ureg.Unit))

@keewis
Copy link
Contributor Author

keewis commented Jan 15, 2020

This looks fine to me, but I'd make unit_registry a keyword-only argument if it should be possible to specify contexts without passing a different unit registry.

If it's fine with you I'd put in a PR so we can comment on the actual code (and documentation).

Also, is_compatible works (it strongly suggests the result is a bool), but I would like to suggest compatible_with (which feels a little bit more natural, especially as a method). No strong opinion, though.

@hgrecco
Copy link
Owner

hgrecco commented Jan 16, 2020

I'd make unit_registry a keyword-only argument if it should be possible to specify contexts without passing a different unit registry.

Agree

If it's fine with you I'd put in a PR so we can comment on the actual code (and documentation).
That would be great. I would also add this function to the registry itself.

Regarding the name, for methods I usualy like using verbs. But check_compatible_with is very long. Any other ideas?

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

Successfully merging a pull request may close this issue.

2 participants