yt-project / unyt

Handle, manipulate, and convert data with units in Python
https://unyt.readthedocs.io
BSD 3-Clause "New" or "Revised" License
363 stars 47 forks source link

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

Open mkhorton opened 4 years ago

mkhorton commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

mkhorton commented 4 years ago

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

ngoldbaum commented 4 years ago

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