Attempt at replacing | with * for creating quantities

[rieder]

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]

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]

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]

Clearly not yet finished :).

[ipelupessy]

how does it do with the tests?

[ipelupessy]

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

[rieder]

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]

can you give an example of that?

what happens if you keep the old overload available?

[rieder]

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)
...:
...:
---------------------------------------------------------------------------AttributeErrorTraceback (mostrecentcalllast)
<ipython-input-4-4f3dad978eb6>in<module>---->1 (constants.Rydberg_constant*constants.h*constants.c)
2~/Code/amuse/src/amuse/units/quantities.pyin__mul__(self, other)
125def__mul__(self, other):
126other=to_quantity(other)
-->127returnnew_quantity_nonone(self.number*other.number, (self.unit*other.unit).to_simple_form())
128129__rmul__=__mul__~/Code/amuse/src/amuse/units/quantities.pyin__mul__(self, other)
125def__mul__(self, other):
126other=to_quantity(other)
-->127returnnew_quantity_nonone(self.number*other.number, (self.unit*other.unit).to_simple_form())
128129__rmul__=__mul__~/Code/amuse/src/amuse/units/quantities.pyinnew_quantity_nonone(value, unit)
1215 :returns: newScalarQuantityorVectorQuantityobject1216 """
->1217ifnotunit.base:
1218ifisinstance(value, __array_like):
1219returnnumpy.asarray(value) *unit.factorAttributeError: 'ScalarQuantity'objecthasnoattribute'base'

[rieder]

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.

[rieder]

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**-1In [8]: cOut[8]: quantity<299792458.0s**-1m>In [9]: c.unitOut[9]: unit<m>In [10]: c.numberOut[10]: quantity<299792458.0s**-1>In [11]: c=299792458.0* (m*s**-1)
In [12]: cOut[12]: quantity<299792458.0m*s**-1>In [13]: c.unitOut[13]: unit<m*s**-1>In [14]: c.numberOut[14]: 299792458.0

[ipelupessy]

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]

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]

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]

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]

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]

ah, of course that would be __rmul__...

[rieder]

This also fails:

In [31]: 2.0*2.0*kg*m**2Out[31]: quantity<4.0m**2kg>In [32]: 2.0*2.0*kg**2*m**2---------------------------------------------------------------------------AttributeErrorTraceback (mostrecentcalllast)
<ipython-input-32-7f60638bbeb4>in<module>---->12.0*2.0*kg**2*m**2~/Code/amuse/src/amuse/units/quantities.pyin__mul__(self, other)
125def__mul__(self, other):
126other=to_quantity(other)
-->127returnnew_quantity_nonone(self.number*other.number, (self.unit*other.unit).to_simple_form())
128129__rmul__=__mul__~/Code/amuse/src/amuse/units/quantities.pyinnew_quantity_nonone(value, unit)
1214 :returns: newScalarQuantityorVectorQuantityobject1215 """
->1216ifnotunit.base:
1217ifisinstance(value, __array_like):
1218returnnumpy.asarray(value) *unit.factorAttributeError: 'ScalarQuantity'objecthasnoattribute'base'

also:

In [33]: (2.0*2.0*kg*m**2).unitOut[33]: unit<kg>In [34]: (2.0*2.0*kg*m**2).numberOut[34]: quantity<4.0m**2>

[ipelupessy]

hmm, not sure this goes ok....

[rieder]

it's really tricky...

[rieder]

This is what's happening internally:

In [2]: 1.0* (kg*m)
mul (kg, m) # self, otherrmul (kg*m, 1.0) # self, otherOut[2]: quantity<1.0kg*m>In [3]: 1.0*kg*mrmul (kg, 1.0)
rmul (m, 1.0)
mul (kg, none)
Out[3]: quantity<1.0mkg>

[rieder]

And

In [4]: 1.0*kg**2*mrmul (kg**2, 1.0)
rmul (m, 1.0)
mul (kg**2, none)
rmul (kg**2, 1)
---------------------------------------------------------------------------AttributeErrorTraceback (mostrecentcalllast)```

[rieder]

In astropy:

In [9]: 1.0*u.kg**2*u.mrmulkg21.0mulmkg2Out[9]: <Quantity1.0kg2m>```

[ipelupessy]

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..

[rieder]

I see

[rieder]

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

[ipelupessy]

btw ignore the review, these are just comments

[rieder]

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

[ipelupessy]

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

[rieder]

Yes, changing the order seems to work:

In [12]: 3.206361533e-53*C**3*m**3*J**-2---------------------------------------------------------------------------AttributeErrorTraceback (mostrecentcalllast)
<ipython-input-12-545b55826c72>in<module>---->13.206361533e-53*C**3*m**3*J**-2~/Code/amuse/src/amuse/units/quantities.pyin__mul__(self, other)
125def__mul__(self, other):
126other=to_quantity(other)
-->127returnnew_quantity_nonone(self.number*other.number, (self.unit*other.unit).to_simple_form())
128129__rmul__=__mul__~/Code/amuse/src/amuse/units/core.pyinto_simple_form(self)
211result=result*base212else:
-->213result=result* (base**n)
214215returnresult~/Code/amuse/src/amuse/units/quantities.pyin__mul__(self, other)
125def__mul__(self, other):
126other=to_quantity(other)
-->127returnnew_quantity_nonone(self.number*other.number, (self.unit*other.unit).to_simple_form())
128129__rmul__=__mul__~/Code/amuse/src/amuse/units/quantities.pyinnew_quantity_nonone(value, unit)
1214 :returns: newScalarQuantityorVectorQuantityobject1215 """
->1216ifnotunit.base:
1217ifisinstance(value, __array_like):
1218returnnumpy.asarray(value) *unit.factorAttributeError: 'ScalarQuantity'objecthasnoattribute'base'In [13]: C**3*m**3*J**-2*3.206361533e-53Out[13]: quantity<3.206361533e-53C**3*m**3*J**-2>

[ipelupessy]

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

[ipelupessy]

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]

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]

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]

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]

keep open

[stale]

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.

[stale]

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