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

Refining exceptions #346

Closed
neutrinoceros opened this issue Jan 2, 2023 · 3 comments · Fixed by #416
Closed

Refining exceptions #346

neutrinoceros opened this issue Jan 2, 2023 · 3 comments · Fixed by #416
Assignees

Comments

@neutrinoceros
Copy link
Member

To keep track of this important comment from @ngolbaum

I'm not really a fan of UnytError but I also don't think that should block getting the __array_function__ stuff working. I wish this was raising UnitOperationError, or we somehow made UnitOperationError more general since I would guess that's the most common type of exception people would be catching for this sort of thing and it irks me a bit that they'd need to catch more than one kind of exception for different corner cases.

We probably need to more carefully look at how exceptions work in unyt in general since right now the situation is kind of a hodgepodge, although that might need a deprecation cycle since we'd be doing an API break.

For now I'm just going to merge this, but I'd like to have a discussion about how to handle exceptions, whether we need to do some sort of deprecation cycle, and how we can make it simpler to deal with exceptions raised by unyt before we do the final release.

Originally posted by @ngoldbaum in #338 (comment)

@neutrinoceros
Copy link
Member Author

So the reason I created UnytError is that I needed a simple error class for numpy.cumprod that didn't fit any existing custom exception. The error message also doesn't match UnitOperationError, as no operator or secondary unit is directly involved. A strategy we've used elsewhere was to subclass UnitOperationError, but I'm hesitant to do this for just one error (what should it even be named ? UnytCumprodError ?). Instead, I keep circling back to thinking having a single generic error type like UnytError has value since it allows for unique-yet-custom error messages to be crafted as we need, leaves room for deduplication if said error messages need repeating in the future (we can subclass it without breaking code relying on it), and provides a unique error that can be expected downstream (provided we revise all other exceptions to descend from it).

@ngoldbaum
Copy link
Member

I thought about this a bit today and I think I’m ok with UnytError, as long as it inherits from Exception and the other custom exceptions in unyt inherit from it instead of Exception. This was we minimally break downstream code at the cost of some messiness in out exceptions. Ideally we would have thought the exceptions through more carefully but the ship sailed on that I think.

To be actually useful as the “generic unyt exception” class it’ll need some docs, probably in the generic unyt usage section, as well as a cross-reference or different text in the section on using unyt with a larger python library. It’ll also need a call-out in the release notes when that goes out.

Does that all sound reasonable?

@neutrinoceros
Copy link
Member Author

Sounds like a plan to me. I think I can do all of that in the next few days.

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