[WIP] Addition of `UnitOperationWarning` and `strict` kwarg to `UnitRegistry` #166

mkhorton · 2020-08-05T19:33:54Z

As discussed in #165

This adds a UnitOperationWarning and a new strict kwarg to UnitRegistry, so that a custom UnitRegistry can be used that will gracefully fall-back to returning the output of an operation without units if a UnitOperationError would otherwise have been raised.

Currently have a demo working, however it's failing test_ufunc specifically, because it seems like the u0 or u1 in this case don't have a .registry or .dimensions attributes despite being a unyt_array. Suggestions here welcome, I'm not sure why this is the case.

Once it's working I can add some additional tests and check style.

mkhorton · 2020-08-05T19:48:13Z

The second aspect of this that would be nice to address:

As written, this PR will always issue a warning. Really, we only want the warning if units on one side of the operator are not specified and assumed dimensionless. If units are actually provided on both sides of the operator, it seems sensible to still raise the UnitOperationError and not the warning.

Unfortunately I can't see a way to figure out if the units are intended (i.e. a unyt_array that was intentionally created as dimensionless) or the units are dimensionless simply because they weren't specified and were assumed, i.e. as in:

unyt/unyt/array.py

Line 562 in 14e4cdb

# Nothing provided. Make dimensionless...

Edit: the only option that occurs to me here is a new unit, assumed_dimensionless, but that would be a major change, or an extra bit of metadata on Unit

mkhorton · 2020-09-23T21:37:02Z

Would a unyt maintainer be willing to help me understand why test_ufunc is failing in this PR? Otherwise the functionality in this PR is fairly complete.

ngoldbaum · 2020-09-23T22:21:34Z

Hey I’ll try to take a look at this soon. Sorry for the radio silence, it’s hard for me to focus on programming tasks these days...

mkhorton · 2020-09-23T22:29:05Z

Many thanks @ngoldbaum, and no worries at all. Grateful for any feedback

neutrinoceros · 2021-08-16T09:05:02Z

Hey @mkhorton, I can help you with this. Since there is a conflict and unyt's CI was migrated to a different system, I'd advise you rebase this branch if you still want to work on this PR. Sorry it's been a while !

mkhorton · 2021-08-16T15:55:42Z

Hi @neutrinoceros, thanks for the offer to help! Would be very happy to see this functionality added.

I've resolved the current merge conflict and this branch is now up to date.

neutrinoceros · 2021-08-16T16:00:43Z

Alright, seems like there are 3 failures, 2 of which are code-style-related. Can you run black + flake8 and fix the warnings you get from the latter ? Then all that should be left is the ufunc issue.

neutrinoceros · 2021-08-16T16:35:13Z

Sorry the infrastructure isn't very well specified yet, it looks like you ran a different version of black that's conflicting with the latest. It'd be preferable to rebase and force push your branch now, to avoid tarnishing the history of files affected by cancelling changes. If you're not confortable with the process I can do it for you, just let me know.

mkhorton · 2021-08-16T21:08:29Z

Ok, I rebased, by all means make changes directly to my fork if preferred. I don't mind installing a specific version of black if necessary too.

neutrinoceros · 2021-08-16T21:14:41Z

We can always squash and rebase later to minimise disrupting your workflow. In the mean time I've pushed a single commit to fix the formatting issue. It is getting late in my timezone, I'll have a deeper look at the actual content tomorrow and hopefully we can move forward with this ufunc issue. Thank you so much for your patience.

neutrinoceros

Okay I think I found the fix to your problem. I also have a couple suggestions about string formatting an other small nitpicks. I'd like to review this again after my suggestions are dealt with. Thanks again for your patience, really appreciate it.

unyt/array.py

unyt/exceptions.py

unyt/tests/test_unyt_array.py

unyt/array.py

unyt/unit_registry.py

unyt/array.py

Co-authored-by: Clément Robert <[email protected]>

neutrinoceros

So I must say I do not disapprove of the idea, but I have mixed feelings about the current implementation. I'm thinking that Python has a builtin mechanism to change warnings into errors (see https://docs.python.org/3/using/cmdline.html#cmdoption-w), so maybe it's worth considering an alternative implementation where we always raise warnings instead of errors for invalid operations, and so developers of projects that consume the unyt library can choose when they want to turn the -Werror flag on in their CI (note that pytest, ipython and other CI relevant interpreters have support for -W and pass it down to python). In my opinion this alternative is worth considering, but it needs further discussion.
@ngoldbaum @jzuhone @matthewturk please weigh in.

unyt/unit_registry.py

neutrinoceros · 2021-08-18T07:04:11Z

unyt/array.py

@@ -166,6 +167,20 @@ def _iterable(obj):
    return True


+def _unit_operation_error_raise_or_warn(ufunc, u0, u1, func, *inputs):


I am hesitant about how this function implements the proposed functionality. Specifically, it feels weird to replace a raise statement with a return statement (where this function is used).
Another comment is that the func argument is redundant (I think), since it can be obtained as func = ufunc.method.

I share this uneasiness but I'm not sure of a better solution. It seems clear to me that some users will want this to fail explicitly and raise, whereas others will just want the warning, so there has to be a conditional somewhere that switches between that behaviour?

To your second comment, makes sense, I'll remove the func argument.

Make new argument keyword only Co-authored-by: Clément Robert <[email protected]>

neutrinoceros · 2021-08-19T09:21:03Z

@mkhorton Looks like both of your latest commits broke some tests, can you revert back to the last working state ?
should be as simple as

git reset --hard HEAD~2
git push --force

mkhorton · 2021-08-25T20:46:13Z

So I must say I do not disapprove of the idea, but I have mixed feelings about the current implementation. I'm thinking that Python has a builtin mechanism to change warnings into errors (see https://docs.python.org/3/using/cmdline.html#cmdoption-w), so maybe it's worth considering an alternative implementation where we always raise warnings instead of errors for invalid operations, and so developers of projects that consume the unyt library can choose when they want to turn the -Werror flag on in their CI (note that pytest, ipython and other CI relevant interpreters have support for -W and pass it down to python). In my opinion this alternative is worth considering, but it needs further discussion.
@ngoldbaum @jzuhone @matthewturk please weigh in.

Thanks @neutrinoceros, I was unaware of this functionality, I would also be in favour of this alternative since it's officially-supported by Python but will have to do some reading. Despite writing Python for quite a while, I feel like I'm still learning a lot :-)

I would also be grateful for any additional comments from other maintainers if anyone has thoughts on this functionality, especially if anyone has strong objections.

mkhorton and others added 7 commits August 16, 2021 13:30

Demo implementation of warnings instead of UnitOperationError

7a1553f

Formalize with UnitOperationWarning, add kwarg to UnitRegistry

fb05c4d

Add minimal test to start (fails test_ufuncs)

152ba95

Add usage documentation

b433c19

Fix typo

ed298ad

black formatting and flake8 edits

b19e139

black formatting

7230006

mkhorton force-pushed the warnings branch from 0cc7d62 to 7230006 Compare August 16, 2021 21:05

formatting with black 21.7b0

04e48f7

fix a formatting error

05f0763

neutrinoceros requested changes Aug 17, 2021

View reviewed changes

mkhorton and others added 8 commits August 17, 2021 14:20

Import ordering

d64b089

Favour f-strings

0d0bf7d

Co-authored-by: Clément Robert <[email protected]>

Ensure space is added to error message

be41182

Co-authored-by: Clément Robert <[email protected]>

Favor f-string and ' as delimiter

e5b139a

Co-authored-by: Clément Robert <[email protected]>

Import ordering

0943e71

Remove unused variable

b15d0ad

Fix grammar

2533b4f

Co-authored-by: Clément Robert <[email protected]>

Fix conditional and if statement

d3436be

Co-authored-by: Clément Robert <[email protected]>

neutrinoceros reviewed Aug 18, 2021

View reviewed changes

neutrinoceros linked an issue Aug 18, 2021 that may be closed by this pull request

Graceful fallback for unit errors (warning, casting to float/array, etc.) rather than raising UnitOperationError? #165

Open

neutrinoceros and others added 3 commits August 18, 2021 15:01

fix a typo

f87b4bf

fix a typo

8f20ac7

Update unyt/unit_registry.py

75e89d3

Make new argument keyword only Co-authored-by: Clément Robert <[email protected]>

mkhorton force-pushed the warnings branch from eaeb2d1 to 75e89d3 Compare August 25, 2021 20:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Addition of `UnitOperationWarning` and `strict` kwarg to `UnitRegistry` #166

[WIP] Addition of `UnitOperationWarning` and `strict` kwarg to `UnitRegistry` #166

mkhorton commented Aug 5, 2020 •

edited

Loading

mkhorton commented Aug 5, 2020 •

edited

Loading

mkhorton commented Sep 23, 2020

ngoldbaum commented Sep 23, 2020

mkhorton commented Sep 23, 2020

neutrinoceros commented Aug 16, 2021

mkhorton commented Aug 16, 2021

neutrinoceros commented Aug 16, 2021

neutrinoceros commented Aug 16, 2021

mkhorton commented Aug 16, 2021

neutrinoceros commented Aug 16, 2021

neutrinoceros left a comment

neutrinoceros left a comment •

edited

Loading

neutrinoceros Aug 18, 2021

mkhorton Aug 18, 2021

mkhorton Aug 18, 2021

neutrinoceros commented Aug 19, 2021 •

edited

Loading

mkhorton commented Aug 25, 2021

		@@ -166,6 +167,20 @@ def _iterable(obj):
		return True


		def _unit_operation_error_raise_or_warn(ufunc, u0, u1, func, *inputs):

[WIP] Addition of UnitOperationWarning and strict kwarg to UnitRegistry #166

Are you sure you want to change the base?

[WIP] Addition of UnitOperationWarning and strict kwarg to UnitRegistry #166

Conversation

mkhorton commented Aug 5, 2020 • edited Loading

mkhorton commented Aug 5, 2020 • edited Loading

mkhorton commented Sep 23, 2020

ngoldbaum commented Sep 23, 2020

mkhorton commented Sep 23, 2020

neutrinoceros commented Aug 16, 2021

mkhorton commented Aug 16, 2021

neutrinoceros commented Aug 16, 2021

neutrinoceros commented Aug 16, 2021

mkhorton commented Aug 16, 2021

neutrinoceros commented Aug 16, 2021

neutrinoceros left a comment

Choose a reason for hiding this comment

neutrinoceros left a comment • edited Loading

Choose a reason for hiding this comment

neutrinoceros Aug 18, 2021

Choose a reason for hiding this comment

mkhorton Aug 18, 2021

Choose a reason for hiding this comment

mkhorton Aug 18, 2021

Choose a reason for hiding this comment

neutrinoceros commented Aug 19, 2021 • edited Loading

mkhorton commented Aug 25, 2021

[WIP] Addition of `UnitOperationWarning` and `strict` kwarg to `UnitRegistry` #166

[WIP] Addition of `UnitOperationWarning` and `strict` kwarg to `UnitRegistry` #166

mkhorton commented Aug 5, 2020 •

edited

Loading

mkhorton commented Aug 5, 2020 •

edited

Loading

neutrinoceros left a comment •

edited

Loading

neutrinoceros commented Aug 19, 2021 •

edited

Loading