-
Notifications
You must be signed in to change notification settings - Fork 49
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
Comments
Naively, looking at the code, I'd think something like replacing:
with:
might be sufficient, but I'm not sure if this would be missing important consequences or edge-cases (edited for correctness). |
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. |
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 |
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.
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? |
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. |
Thanks! Ok, I'll give it a go and check in if there are any issues. |
Are work-in-progress/draft PRs ok here? |
Yup, ping me if you have questions or need a hand. |
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:
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 return4
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.The text was updated successfully, but these errors were encountered: