ENH: (NEP 18) test and implement combinatory functions #351
Conversation
neutrinoceros
force-pushed
the
nep18_masks
branch
2 times, most recently
from
ff8639c
to
730eb51
Compare
There was a problem hiding this comment.
I'm worried that some of the new errors that will get raised here will break downstream code in a somewhat annoying way. I would err on the side of only checking unit consistency if one of the arguments has a unit, otherwise if all arguments are numbers or ndarrays, assume that the arguments have units appropriate for the array.
| def test_clip_mixed_units(): | ||
| a = [1, 2, 3, 4, 5, 6] * cm | ||
| with pytest.raises(UnitInconsistencyError): | ||
| np.clip(a, 3 * cm, 4) |
There was a problem hiding this comment.
I agree this should error, but I kinda think np.clip(a, 3, 4) shouldn't error. The fact that it does now breaks existing unyt tests is another indicator that it shouldn't error, since it could just as easily break downstream user code.
There was a problem hiding this comment.
but I kinda think np.clip(a, 3, 4) shouldn't error
me too TBH, but in that case it seems to me that making np.clip(a, 3 * cm, 4) error will add more complexity than it is worth.
There was a problem hiding this comment.
nevermind it's actually quite simple
That is also how I think it should work, and I agree that the fact that I had to change existing tests is indication that it's currently on the wrong track. |
Yes, but again only if all of the arguments are pure numbers. If one of them has units, I think it makes sense to do unit checking. |
neutrinoceros
force-pushed
the
nep18_masks
branch
from
730eb51
to
0c6a4cf
Compare
|
@ngoldbaum I just pushed a changeset that I think should resolved on-going discussions, however I need to make another pass tomorrow to make sure I've updated all other functions to be in line with the behaviour we want. |
|
Alright this is good for review again. To make changes easier to review I just pushed incremental changes, but |
unyt/testing.py
Outdated
| @@ -49,6 +51,12 @@ def assert_allclose_units(actual, desired, rtol=1e-7, atol=0, **kwargs): | |||
| raise AssertionError | |||
|
|
|||
|
|
|||
| def assert_array_equal_units(a, b): | |||
There was a problem hiding this comment.
Now that it's in the public API it needs a docstring
|
Sorry for not getting to this last friday. Other than the missing docstring this looks good to me. Please go ahead and squash the commits as you'd like :) |
|
No worries ! I'll try to get this finished this evening. |
neutrinoceros
force-pushed
the
nep18_masks
branch
from
0af8f8f
to
7d8d96e
Compare
|
@ngoldbaum I force-pushed with a compact history. The change you requested in now in the second commit. It's now ready for merging. |
Covers most of the functions last disscussed in #289 (comment)