Add custom error types in uxarray#1621
Open
Sevans711 wants to merge 4 commits into
Open
Conversation
Adds DataCenteringError, DimensionError, GridInvalidError, and GridsMismatchError. Replaces existing errors with one of these where appropriate AND where it will not break backwards compatibility. Kept notes (will include in PR writeup) about places where it would break compatibility, and did not change those places yet.
spot check that custom error types are working: changes two existing tests to catch custom error types. Does not change many other tests, because it is also desirable to confirm that these changes are actually backwards-compatible, by not messing with the test suite too much.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1556
Overview
Adds custom error types in uxarray. Defining custom error types can help with:
In particular, this PR adds the following error types:
Small expansion of scope
This PR expands scope slightly beyond that of the original issue as follows:
YacNotAvailableErrorfrom uxarray/remap/yac.py to uxarray/errors.py.The following justification for this move is provided in the docstring at the top of errors.py:
Though, this could be reverted if PR reviewers disagree or feel that this should be scoped as a separate issue instead.
Compatibility notes:
This PR should be fully backwards compatible, or nearly fully backwards compatible. Compatibility is maintained via subclassing. For example, any code catching ValueError will still catch any of the newly-defined custom error types, because they inherit from ValueError.
This PR adjusts two tests to check that custom error types are indeed being raised (e.g.
with pytest.raises(DataCenteringError)instead ofwith pytest.raises(ValueError)). It does not adjust all tests to the new error types because it is desirable to see that the existing tests still pass, to confirm there are no breaking changes here.Thinking about it a bit further, there are three technically-possible breakage points for existing code, but only for hopefully-very-unlikely code design patterns:
repr(err).startswith("ValueError")type(err) == ValueErrorisinstance(err, Exception) and not isinstance(err, ValueError), could fail in some cases. This PR replaces Exception with a custom error type in some cases, such as using DataCenteringError instead of Exception in a few places in grid.py.Other Recommended Changes (Not Implemented Here)
Should custom errors become part of the public API? I would be interested in at least adding
import uxarray.errorsinuxarray.__init__.py, so that users who have imported uxarray already can do things like:ux.errors.DimensionErrorwithout needing to remember to add another import statement likefrom uxarray.errors import DimensionError.There are a few locations where the new error types would better describe the actual error, but which are not included in this PR because they could break backwards compatibility. These could be added here if PR reviewers want, or split into a separate issue and PR afterwards. Including these here is not necessary to close issue #1556.
I also put together a variety of error-type changes I would recommend but which are unrelated to this PR, because they do not use the new error types. (Example: ValueError → TypeError in grid.utils.make_setter: f"{key} must be an xr.DataArray", which gets raised if not isinstance(value, xr.DataArray).) I have opened a separate issue to track those, see #1622.
PR Checklist
General
Testing
Documentation
_) and have been added todocs/internal_api/index.rstdocs/user_api/index.rst