pint icon indicating copy to clipboard operation
pint copied to clipboard

Offset Unit Multiplication

Open jules-ch opened this issue 2 years ago • 5 comments

Just a thought here.

We had a lot of questions regarding offset units particularly when using multiplication with unit.

Like :

15 * ureg.deg_C
Raising an error instead of Quantity(15, "deg_C")

In the same direction, I do remember @jthielen & @dopplershift having performance issues in metpy when using this form to initialize Quantity. And had to fallback using the constructor.

Wouldn't be possible to have this form be equivalent to initializing with the Quantity constructor:

currently : 15 * ureg.deg_C <=> Quantity(1, "deg_C") * 15 to : 15 * ureg.deg_C <=> Quantity(15, "deg_C")

We will get rid of the performance issues & any confusion when using this simple form. 2 birds, 1 stone.

Regarding security, we would still have those offset checks when making any multiplication or division with:

  • Quantity & Units
  • Quantity & Quantity

In the documentation, this sentence still stands with this change & will only affect the simple form of value * unit. Any further operation would fall under the rule of Quantity multiplication.

However, multiplication, division and exponentiation of quantities with offset units is problematic just like addition. Pint (since version 0.6) will by default raise an error when a quantity with offset unit is used in these operations. Due to this quantities with offset units cannot be created like other quantities by multiplication of magnitude and unit but have to be explicitly created

Any thoughts @hgrecco, @keewis or use cases I haven't thought about ? When making this change, I see no test failing.

jules-ch avatar Nov 18 '21 21:11 jules-ch

The current implementation:



    def __mul__(self, other):
        if self._check(other):
            if isinstance(other, self.__class__):
                return self.__class__(self._units * other._units)
            else:
                qself = self._REGISTRY.Quantity(1, self._units)
                return qself * other

        if isinstance(other, Number) and other == 1:
            return self._REGISTRY.Quantity(other, self._units)

        return self._REGISTRY.Quantity(1, self._units) * other

And as you can see 1 * ureg.degC will return when magnitude is 1. 😄

jules-ch avatar Nov 18 '21 21:11 jules-ch

I would strongly disfavour making multiplication a synonym with construction for offset units. There is no physical meaning to multiplication with what we call here "offset units" and are really quantity coordinates. The multiplication sign intuitively represents an associative operation, but with the proposed changes you could do (2 * 4) * u.deg_C but not 2 * (4 * u.deg_C). If you have generic code that is constructing a quantity from unit and value, you can always do u.Quantity(value,unit), that is explicit and unambiguous.

I wasn't aware that there is a special-case for "1" though. What is the reason for that special case? That feels like a hacky work-around for some other fundmental problem/misconception. I can see all sorts of potential subtle bugs and surprises with that special case. I think that one should be removed.

burnpanck avatar Nov 28 '21 11:11 burnpanck

Btw, I have an open PR to enable parsing of offset units from strings, and that one only allows the form without the multiplication.

burnpanck avatar Nov 28 '21 11:11 burnpanck

What about removing offset-unit multiplication completely and accept only the explicit creation via u.Quantity(value,unit)? This would solve the problem at the basis. Of course such a change should not happen from one version to the next.

dalito avatar Nov 28 '21 11:11 dalito

I am all in favour of removing/disallowing multiplication of offset units under any circumstance. However, I'm not sure I understand what exactly you are suggesting here. It is currently already disallowed completely, except for a registry flag that re-enables such multiplications. And then there is the special case above for numeric values equal to 1. If you mean to remove that special case, that's what I'm saying.

burnpanck avatar Nov 28 '21 12:11 burnpanck