Attempt at replacing | with * for creating quantities #693

rieder · 2020-10-28T15:45:19Z

See issue #687. This PR makes it possible to construct quantities using the * operator, which would previously have resulted in a unit.
With this PR, a unit multiplied/divided by another unit will still give a unit, while other multiplications/divisions will result in a quantity. Of course, quantities can still be converted to units with .to_unit().

rieder · 2020-10-28T15:47:39Z

Using the | operator to create quantities is still possible with this PR by the way - but if merged, we should add a deprecation warning to that method.

rieder · 2020-10-28T15:55:17Z

I think the impact should be minimal with this PR - only scripts that construct new units will need modification, and I don't recall ever writing such a script myself.

rieder · 2020-10-28T16:00:17Z

Clearly not yet finished :).

ipelupessy · 2020-10-28T16:12:23Z

how does it do with the tests?

ipelupessy · 2020-10-28T16:14:46Z

its very hard to review with the zillion formatting changes... [its not a good sign when github refuses to load the diff]

rieder · 2020-10-28T16:15:03Z

Lots of errors so far, working on fixing them.
One problem is that the order of constructing units/quantities needs to be taken into account.
Not sure what we can do about that except advising that any unit should be constructed first before assigning it to a value, using parentheses or something.

ipelupessy · 2020-10-28T16:16:36Z

can you give an example of that?

what happens if you keep the old overload available?

rieder · 2020-10-28T16:18:18Z

it's still available, but some constructed units are now suddenly quantities.
Leads to errors like this:

In [4]: (constants.Rydberg_constant * constants.h * constants.c)
   ...:
   ...:
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-4-4f3dad978eb6> in <module>
----> 1 (constants.Rydberg_constant * constants.h * constants.c)
      2

~/Code/amuse/src/amuse/units/quantities.py in __mul__(self, other)
    125     def __mul__(self, other):
    126         other = to_quantity(other)
--> 127         return new_quantity_nonone(self.number * other.number, (self.unit * other.unit).to_simple_form())
    128
    129     __rmul__ = __mul__

~/Code/amuse/src/amuse/units/quantities.py in __mul__(self, other)
    125     def __mul__(self, other):
    126         other = to_quantity(other)
--> 127         return new_quantity_nonone(self.number * other.number, (self.unit * other.unit).to_simple_form())
    128
    129     __rmul__ = __mul__

~/Code/amuse/src/amuse/units/quantities.py in new_quantity_nonone(value, unit)
   1215     :returns: new ScalarQuantity or VectorQuantity object
   1216     """
-> 1217     if not unit.base:
   1218         if isinstance(value, __array_like):
   1219             return numpy.asarray(value) * unit.factor

AttributeError: 'ScalarQuantity' object has no attribute 'base'

rieder · 2020-10-28T16:26:51Z

its very hard to review with the zillion formatting changes... [its not a good sign when github refuses to load the diff]

should be a bit easier now. Main changes are in core.py.

src/amuse/units/derivedsi.py

rieder · 2020-10-28T19:47:03Z

can you give an example of that?

c = 299792458.0 * (m*s**-1) is fine, but c = 299792458.0 * m*s**-1 is not:

In [7]: c = 299792458.0 * m*s**-1

In [8]: c
Out[8]: quantity<299792458.0 s**-1 m>

In [9]: c.unit
Out[9]: unit<m>

In [10]: c.number
Out[10]: quantity<299792458.0 s**-1>

In [11]: c = 299792458.0 * (m*s**-1)

In [12]: c
Out[12]: quantity<299792458.0 m * s**-1>

In [13]: c.unit
Out[13]: unit<m * s**-1>

In [14]: c.number
Out[14]: 299792458.0

ipelupessy · 2020-10-28T20:23:49Z

does the ror still work after this?

maybe is also good to take stock of all the implications that this change has (issue?):

old simulation scipts
documentation
book
sandbox
downstream (dependend packages ie omuse)
etc

rieder · 2020-10-28T20:45:15Z

Yes, the ror is untouched and still works.

maybe is also good to take stock of all the implications that this change has (issue?): old simulation scipts documentation book sandbox downstream (dependend packages ie omuse) etc

Sure. There must not be any remaining issues when this is merged*, so we'll need to test this extensively. *: other than the creation of new units, which can by design not be done in the old way with this change...

ipelupessy · 2020-10-29T08:22:59Z

quantities has a as_unit and a to_unit - they are slightly different...don't know why.. (i prefer the name to_unit, don't know about implementation yet)

also - there are a bunch of as_units that can be removed and I think in the constants definition we can also remove '* none' units

rieder · 2020-10-29T10:46:16Z

Another issue:
1.0 * unit will return the unit. This should probably return a quantity instead (in line with previous behaviour and other unit systems)

rieder · 2020-10-29T10:59:58Z

Another issue:
1.0 * unit will return the unit. This should probably return a quantity instead (in line with previous behaviour and other unit systems)

units.kg.__mul__(1.0) does return a quantity. Apparently with 1.0 * units.kg this isn't even called.

rieder · 2020-10-29T11:02:14Z

ah, of course that would be __rmul__...

rieder · 2020-10-29T11:08:05Z

This also fails:

In [31]: 2.0 * 2.0 * kg * m**2
Out[31]: quantity<4.0 m**2 kg>

In [32]: 2.0 * 2.0 * kg**2 * m**2
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-32-7f60638bbeb4> in <module>
----> 1 2.0 * 2.0 * kg**2 * m**2

~/Code/amuse/src/amuse/units/quantities.py in __mul__(self, other)
    125     def __mul__(self, other):
    126         other = to_quantity(other)
--> 127         return new_quantity_nonone(self.number * other.number, (self.unit * other.unit).to_simple_form())
    128
    129     __rmul__ = __mul__

~/Code/amuse/src/amuse/units/quantities.py in new_quantity_nonone(value, unit)
   1214     :returns: new ScalarQuantity or VectorQuantity object
   1215     """
-> 1216     if not unit.base:
   1217         if isinstance(value, __array_like):
   1218             return numpy.asarray(value) * unit.factor

AttributeError: 'ScalarQuantity' object has no attribute 'base'

also:

In [33]: (2.0 * 2.0 * kg * m**2).unit
Out[33]: unit<kg>

In [34]: (2.0 * 2.0 * kg * m**2).number
Out[34]: quantity<4.0 m**2>

ipelupessy · 2020-10-29T11:12:27Z

hmm, not sure this goes ok....

rieder · 2020-10-29T11:12:50Z

it's really tricky...

rieder · 2020-10-29T11:18:52Z

This is what's happening internally:

In [2]: 1.0 * (kg * m)
mul (kg, m)  # self, other
rmul (kg * m, 1.0)  # self, other
Out[2]: quantity<1.0 kg * m>

In [3]: 1.0 * kg * m
rmul (kg, 1.0)
rmul (m, 1.0)
mul (kg, none)
Out[3]: quantity<1.0 m kg>

rieder · 2020-10-29T11:20:28Z

And

In [4]: 1.0 * kg**2 * m
rmul (kg**2, 1.0)
rmul (m, 1.0)
mul (kg**2, none)
rmul (kg**2, 1)
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)```

rieder · 2020-10-29T11:36:43Z

In astropy:

In [9]: 1.0 * u.kg**2 * u.m
rmul kg2 1.0
mul m kg2
Out[9]: <Quantity 1.0 kg2 m>```

src/amuse/units/core.py

ipelupessy · 2020-10-29T15:22:16Z

src/amuse/units/core.py

-                return factor_unit(other, self)
-            return self.new_quantity(other)
+        if isinstance(other, unit):
+            return factor_unit(other, self)


this is wrong, because factor units are supposed to construct from a number(like) and a unit..you should only get here with other a number..

Replaced it with mul_unit, I think that's correct?

ipelupessy · 2020-10-29T15:23:19Z

btw ignore the review, these are just comments

rieder · 2020-10-29T15:24:14Z

comments are useful, I won't ignore them :)

ipelupessy · 2020-10-29T15:27:43Z

would it work if 1000*m resolves to a quantity and m*1000 to a unit?

rieder · 2020-10-29T15:30:09Z

Yes, changing the order seems to work:

In [12]: 3.206361533e-53 * C**3*m**3*J**-2
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-12-545b55826c72> in <module>
----> 1 3.206361533e-53 * C**3*m**3*J**-2

~/Code/amuse/src/amuse/units/quantities.py in __mul__(self, other)
    125     def __mul__(self, other):
    126         other = to_quantity(other)
--> 127         return new_quantity_nonone(self.number * other.number, (self.unit * other.unit).to_simple_form())
    128
    129     __rmul__ = __mul__

~/Code/amuse/src/amuse/units/core.py in to_simple_form(self)
    211                     result = result * base
    212             else:
--> 213                 result =  result * (base ** n)
    214
    215         return result

~/Code/amuse/src/amuse/units/quantities.py in __mul__(self, other)
    125     def __mul__(self, other):
    126         other = to_quantity(other)
--> 127         return new_quantity_nonone(self.number * other.number, (self.unit * other.unit).to_simple_form())
    128
    129     __rmul__ = __mul__

~/Code/amuse/src/amuse/units/quantities.py in new_quantity_nonone(value, unit)
   1214     :returns: new ScalarQuantity or VectorQuantity object
   1215     """
-> 1216     if not unit.base:
   1217         if isinstance(value, __array_like):
   1218             return numpy.asarray(value) * unit.factor

AttributeError: 'ScalarQuantity' object has no attribute 'base'

In [13]: C**3*m**3*J**-2 * 3.206361533e-53
Out[13]: quantity<3.206361533e-53 C**3 * m**3 * J**-2>

ipelupessy · 2020-10-29T15:33:31Z

did you see the comment on line 87 of core? (rmul)

ipelupessy · 2020-10-29T15:41:11Z

also the check for a numerical factor of exactly one should then maybe go to __mul__ or do something with factor_unit, but that is tricky...

rieder · 2020-10-29T15:41:12Z

did you see the comment on line 87 of core? (rmul)

~~yes, I think it's fixed now. Error is gone at least.~~
edit: no, it's not

ipelupessy · 2021-01-07T16:45:48Z

~~can you make a version w/o formatting changes? its very hard to see where it stands now~~
its not so bad actually, though its a bit annoying to combine formatting with nonformatting changes...in any case have you looked at this further?

stale · 2022-03-04T15:10:12Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

ipelupessy · 2022-03-05T19:19:46Z

keep open

stale · 2022-05-04T19:49:06Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 28 days if no further activity occurs. Thank you for your contributions.

rieder added 2 commits October 28, 2020 15:41

Attempt at replacing | with * for creating quantities

d1bd861

replace | with * in units package

e56b7ed

rieder requested a review from ipelupessy October 28, 2020 15:45

rieder added 2 commits October 28, 2020 16:19

work in progress

0ca00b8

Merge branch 'master' into units_mul

6c3ba2c

ipelupessy reviewed Oct 28, 2020

View reviewed changes

src/amuse/units/derivedsi.py Show resolved Hide resolved

update

f362b29

add as_unit and to_unit to unit to simplify e.g. named_unit

1a780eb

add test, fix some constants

f97fbdc

1.0 * unit should give a quantity

7b49b90

ipelupessy reviewed Oct 29, 2020

View reviewed changes

seems better

11c5e3d

stale bot added the wontfix label Mar 4, 2022

ipelupessy added keep-open and removed wontfix labels Mar 5, 2022

stale bot added the stale Issues that have been around for a while without updates label May 4, 2022

rieder removed the stale Issues that have been around for a while without updates label May 12, 2022

Attempt at replacing | with * for creating quantities #693

Are you sure you want to change the base?

Attempt at replacing | with * for creating quantities #693

Conversation

rieder commented Oct 28, 2020

rieder commented Oct 28, 2020

rieder commented Oct 28, 2020 • edited

rieder commented Oct 28, 2020

ipelupessy commented Oct 28, 2020

ipelupessy commented Oct 28, 2020 • edited

rieder commented Oct 28, 2020

ipelupessy commented Oct 28, 2020

rieder commented Oct 28, 2020

rieder commented Oct 28, 2020

rieder commented Oct 28, 2020

ipelupessy commented Oct 28, 2020

rieder commented Oct 28, 2020 via email

ipelupessy commented Oct 29, 2020 • edited

rieder commented Oct 29, 2020

rieder commented Oct 29, 2020 • edited

rieder commented Oct 29, 2020

rieder commented Oct 29, 2020 • edited

ipelupessy commented Oct 29, 2020

rieder commented Oct 29, 2020

rieder commented Oct 29, 2020

rieder commented Oct 29, 2020

rieder commented Oct 29, 2020

ipelupessy Oct 29, 2020

Choose a reason for hiding this comment

rieder Oct 29, 2020

Choose a reason for hiding this comment

rieder Oct 29, 2020

Choose a reason for hiding this comment

ipelupessy commented Oct 29, 2020

rieder commented Oct 29, 2020

ipelupessy commented Oct 29, 2020

rieder commented Oct 29, 2020

ipelupessy commented Oct 29, 2020

ipelupessy commented Oct 29, 2020

rieder commented Oct 29, 2020 • edited

ipelupessy commented Jan 7, 2021 • edited

stale bot commented Mar 4, 2022

ipelupessy commented Mar 5, 2022

stale bot commented May 4, 2022

rieder commented Oct 28, 2020 •

edited

ipelupessy commented Oct 28, 2020 •

edited

ipelupessy commented Oct 29, 2020 •

edited

rieder commented Oct 29, 2020 •

edited

rieder commented Oct 29, 2020 •

edited

rieder commented Oct 29, 2020 •

edited

ipelupessy commented Jan 7, 2021 •

edited