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 coercing iterables with nonuniform dimensionally equivalent units to a single unyt_array #211

Merged
merged 2 commits into from
Nov 19, 2021

Conversation

ngoldbaum
Copy link
Member

Fixes #210.

See the test I modified for a usage example.

Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are a couple suggestions. Otherwise looks great to me !

Comment on lines +264 to +265
except UnitConversionError:
raise IterableUnitCoercionError(str(input_object))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my one important suggestion here, see https://www.python.org/dev/peps/pep-3134/

Suggested change
except UnitConversionError:
raise IterableUnitCoercionError(str(input_object))
except UnitConversionError as exc:
raise IterableUnitCoercionError(str(input_object)) from exc

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t see how this is any better. It would help in the future if you would explicitly explain why a change would help instead of suggesting that someone read a long technical document to explain why.

Copy link
Member

@neutrinoceros neutrinoceros Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. This produce a different traceback.

with a bare raise

try:
    1/0
except Exception:
    raise ValueError

one gets

Traceback (most recent call last):
  File "/Users/robcleme/dev/yt-project/yt/t..py", line 2, in <module>
    1/0
ZeroDivisionError: division by zero

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/robcleme/dev/yt-project/yt/t..py", line 4, in <module>
    raise ValueError
ValueError

This reads as if something unexpected happened in the except block, but it's not the case: we're raising a different error very much on purpose.

with a raise from

try:
    1/0
except Exception as exc:
    raise ValueError from exc
Traceback (most recent call last):
  File "/Users/robcleme/dev/yt-project/yt/t..py", line 2, in <module>
    1/0
ZeroDivisionError: division by zero

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/Users/robcleme/dev/yt-project/yt/t..py", line 4, in <module>
    raise ValueError from exc
ValueError

This is preferable because the error message conveys the designed intention better. This is exactly the situation raise from was intended for

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think the second version is any more helpful, sorry. It’s fine as-is.

Comment on lines -164 to +165
>>> from unyt import km, cm, unyt_array
>>> data = [2*cm, 3*km]
>>> from unyt import g, cm, unyt_array
>>> data = [2*cm, 3*g]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice !

@@ -2161,17 +2161,21 @@ def test_ones_and_zeros_like():


def test_coerce_iterable():
from unyt import cm, km
from unyt import cm, m, g
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
from unyt import cm, m, g
from unyt import cm, km, m, g


a = unyt_array([1, 2, 3], "cm")
b = [1 * cm, 2 * km, 3 * cm]
b = [1 * cm, 2 * m, 3 * cm]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems a tiny bit more fragile (in a good way) like this

Suggested change
b = [1 * cm, 2 * m, 3 * cm]
b = [1 * cm, 2 * m, 3 * km]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your suggested change makes zero difference to the actual thing we’re testing, no.

Comment on lines +2170 to +2171
assert_equal(a + b, unyt_array([2, 202, 6], "cm"))
assert_equal(b + a, unyt_array([2, 202, 6], "cm"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert_equal(a + b, unyt_array([2, 202, 6], "cm"))
assert_equal(b + a, unyt_array([2, 202, 6], "cm"))
assert_equal(a + b, unyt_array([2, 202, 300_003], "cm"))
assert_equal(b + a, unyt_array([2, 202, 300_003], "cm"))

@neutrinoceros neutrinoceros added the bug Something isn't working label Nov 19, 2021
@ngoldbaum ngoldbaum merged commit bf8195d into yt-project:master Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants