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

Support rounding of UnitScalars in Python 3 #84

Closed
JCorson opened this issue Apr 16, 2019 · 13 comments
Closed

Support rounding of UnitScalars in Python 3 #84

JCorson opened this issue Apr 16, 2019 · 13 comments

Comments

@JCorson
Copy link
Contributor

JCorson commented Apr 16, 2019

In Python 3, I am seeing the following when attempting to round a UnitScalar.

In [1]: from scimath.units.api import UnitScalar

In [2]: u = UnitScalar(10.1234, units="gram")

In [3]: u
Out[3]: UnitScalar(10.1234, units='0.001*kg')

In [4]: round(u)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-4-dfdf7062be34> in <module>()
----> 1 round(u)

TypeError: type UnitScalar doesn't define __round__ method

Is this expected?

This is with scimath 4.2.0.

@rahulporuri
Copy link
Contributor

rahulporuri commented Apr 16, 2019

It looks like the round built-in function uses the UnitScalar.__int__ method on Python 2 I'm not sure what the round built-in is using on Python 2 but on Python 3, it tries to use the u.__round__ method, which doesn't exist on an UnitScalar object.

@mdickinson
Copy link
Member

mdickinson commented Apr 17, 2019

I'm not 100% convinced that rounding makes sense for values with units, or at least, not in a way that fits the round function: it doesn't make sense to round a length to two decimal places, while it does make sense to round a length to the nearest cm / km / inch / 1/8 inch, etc.

How about a utility function round_to_multiple_of(value, target_unit) (name to be bikeshedded later) that essentially does round(value / target_unit) * target_unit?

@mdickinson
Copy link
Member

mdickinson commented Apr 17, 2019

@JCorson Question about your expectations. If you have:

>>> from scimath.units.api import UnitScalar
>>> u = UnitScalar(12625, units="m")
>>> v = UnitScalar(12.625, units="km")

then u and v are the same length, just with different representations:

>>> u == v
UnitScalar(True, units='None')

What would you want round(u, 2) and round(v, 2) to return in this case? (Or even just round(u) and round(v)?) I'd expect that since u and v are equivalent, the results of round(u, 2) and round(v, 2) should be equivalent, too.

@rahulporuri
Copy link
Contributor

rahulporuri commented Apr 17, 2019

Question about your expectations.

Understanding the usecase will definitely help us in deciding whether or not to support or how to support this functionality.

@mdickinson
Copy link
Member

round of course does make sense in the special case of unit-less quantities, and it wouldn't be unreasonable to support that case.

@JCorson
Copy link
Contributor Author

JCorson commented Apr 17, 2019

Sorry about that, I should have been clearer on the use case. We have been using UnitScalars in the project and many tests use assertAlmostEqual. This worked in Python 2, but the tests are all failing in Python 3 with the aforementioned TypeError. The example code was just to reproduce the exception.

@mdickinson
Copy link
Member

mdickinson commented Apr 17, 2019

I'd consider the fact that this works in Python 2.7 to be a bug in scimath: before we even get to round, this depends on the ability to compare a unitfull quantity with a unitless quantity, which seems to me as though it should be a TypeError. It leads to fun situations like this:

>>> x = UnitScalar(2.0, units="ft")
>>> y = UnitScalar(1.0, units="m")
>>> z = 1.5
>>> x < y
UnitScalar(True, units='None')
>>> y < z
UnitScalar(True, units='None')
>>> z < x
UnitScalar(True, units='None')

The other part of this is due to the existence of __float__, which provides an implicit coercion path that Python 2's round uses but Python 3's round doesn't. (I'd argue that it's questionable to be able to convert a unitfull value to a float, too, but removing that ability would probably break code.)

@mdickinson
Copy link
Member

To be clear, I'm a strong -1 on the idea of supporting round for non-unitless quantities.

@JCorson
Copy link
Contributor Author

JCorson commented Apr 17, 2019

Make sense, and either way works for me. We have already gone through and made all tests use assertEqual so this isn't blocking anything. FTR our use case was not to compare UnitScalars to floats with assertAlmostEqual, but UnitScalars to UnitScalars. So something like:

exp_value = UnitScalar(10.1234, units='gram')

self.assertAlmostEqual(value, exp_value, places=4) # where value = UnitScalar(10.12340000000001, unit='gram')

It is easy enough for us to just right a function to do the comparison that we want.

@mdickinson
Copy link
Member

FTR our use case was not to compare UnitScalars to floats with assertAlmostEqual, but UnitScalars to UnitScalars.

Yep, that's what I assumed you meant. When you do that, the underlying code first subtracts those two UnitScalar values (getting another UnitScalar as the difference), and then compares that difference (or rather the absolute value of the difference) to a float, in this case 1e-4 (from the places = 4). That's the comparison I was talking about that I don't think makes sense.

I'd suggest a custom assertion self.assertEqualToWithin(value1, value2, diff), where all of value1, value2 and diff are comparable "unitful" quantities. E.g., I want to know that my calculated critical dimension is equal to the expected critical dimension to within 1nm, say.

@JCorson
Copy link
Contributor Author

JCorson commented Apr 17, 2019

I'd suggest a custom assertion self.assertEqualToWithin(value1, value2, diff), where all of value1, value2 and diff are comparable "unitful" quantities. E.g., I want to know that my calculated critical dimension is equal to the expected critical dimension to within 1nm, say.

Yup. Sounds good to me.

@JCorson JCorson closed this as completed Apr 17, 2019
@jonathanrocher
Copy link
Contributor

I had some general assertion functions to do what you want I believe so I pushed it as a PR: #85 .

@mdickinson
Copy link
Member

I also opened an issue #86 for the comparison of incomparable quantities.

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

No branches or pull requests

4 participants