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

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

Open
mkhorton opened this issue Aug 5, 2020 · 9 comments · May be fixed by #166

Comments

@mkhorton
Copy link

mkhorton commented Aug 5, 2020

Description

This is a question/feature request, not a bug. If there is a more appropriate place to ask this, please let me know.

We're looking at a way to add comprehensive unit integration into an existing project. This project is already seeing existing use by other downstream code, so we would prefer not to break backwards compatibility abruptly for our users, and would prefer to support a grace period if possible.

Currently, to take an example:

from unyt import Angstrom
a = 3*Angstrom
a + 1  # raises `UnitOperationError`

Is it possible to configure unyt to instead issue a warning here instead of raising and to return a de-unitized value, e.g. for this example to return 4 as a float?

If this is not possible, is there a single place in the unyt code where such a feature could be added?

If this was possible, we would be able to switch to using unyt in our project, but disable strict enforcement of units by default for our users so that we wouldn't break any existing scripts overnight. It'd also allow us to have a more gradual transition period, while simultaneously allowing us to strictly enforce units in our testing and CI and when developing new features.

@mkhorton
Copy link
Author

mkhorton commented Aug 5, 2020

Naively, looking at the code, I'd think something like replacing:

UnitOperationError(ufunc, u0, u1)

with:

def _unit_operation_error_raise_or_warn(ufunc, u0, u1, func, *inputs):
    if STRICT:  # True by default
        raise UnitOperationError(ufunc, u0, u1)
    else:
        warnings.warn("Performing operation without units!")
        unwrapped_inputs = [
            i.value if isinstance(i, unyt_array) or isinstance(i, unyt_quantity)
            else i for i in inputs
        ]
        return func(*unwrapped_inputs)

might be sufficient, but I'm not sure if this would be missing important consequences or edge-cases (edited for correctness).

@ngoldbaum
Copy link
Member

My big concern about this is that if library A and B both depend on unyt, then library A’s choice to turn unit errors into warnings shouldn’t affect library B’s choice. In other words I’m somewhat dubious about a single global flag for controlling this behavior.

One way to do this that could work is to make this controllable at the level of the unit registry object. Your library could then create its own set of units that use a custom unit registry that then controls whether or not an error gets raised.

@ngoldbaum
Copy link
Member

See this section of the unyt docs for more drtails about wrapping unyt: https://unyt.readthedocs.io/en/stable/usage.html#integrating-unyt-into-a-python-library

@ngoldbaum
Copy link
Member

By the way, any new changes need tests and new functionality need docs, I likely won’t review pull requests implementing this unless there are docs and tests.

@mkhorton
Copy link
Author

mkhorton commented Aug 5, 2020

By the way, any new changes need tests and new functionality need docs, I likely won’t review pull requests implementing this unless there are docs and tests.

Sure, that's not a problem if you would be interested in accepting the feature. I saw unyt took pride in its test coverage in the JOSS paper :) I wasn't sure if the feature would be accepted even in principle before putting a significant effort into it myself.

One way to do this that could work is to make this controllable at the level of the unit registry object. Your library could then create its own set of units that use a custom unit registry that then controls whether or not an error gets raised.

That sounds sensible, I agree about concerns about a global flag.

With regards to the feature itself, is the code snippet above the way to go? Or would you expect this might create other issues, or are there other errors other than UnitOperationError that should also be treated as warnings, etc?

@ngoldbaum
Copy link
Member

Your suggestion seems fine, although you’ll also need to figure out how to get the flag from the unit registry and add the flag metadata to the registry object, its initializer, and its documentation.

If you come across other issues or concerns as you look through the codebase just open a new issue or pull request.

@mkhorton
Copy link
Author

mkhorton commented Aug 5, 2020

Thanks! Ok, I'll give it a go and check in if there are any issues.

@mkhorton
Copy link
Author

mkhorton commented Aug 5, 2020

Are work-in-progress/draft PRs ok here?

@ngoldbaum
Copy link
Member

Yup, ping me if you have questions or need a hand.

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

Successfully merging a pull request may close this issue.

2 participants