-
-
Notifications
You must be signed in to change notification settings - Fork 380
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
Add nicer exception message for mistake in coords argument #236
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
As an aside, there's lots of reorganizing to do - most functions in utils
, plot_utils
and xarray_utils
should probably be in a more general spot (data_utils
?), but that shouldn't happen here.
arviz/plots/plot_utils.py
Outdated
return data.sel(**coords) | ||
|
||
except ValueError as err: | ||
raise ValueError("Verify coords keys. {}".format(err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you verify that the stack trace is still informative? I think the pattern is
except ValueError as err:
raise ValueError("Verify coords keys") from err
I might also print out more information - like "make sure {list(coords.keys()} match with the data data_vars ({list(data.data_vars)})"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do from err the text from the original exception is lost. I was keeping it in because it was informative as to which coordinates were missing.
But I agree with your second statement which means the above point will be irrelevant! I'll make the changes and post another example.
arviz/plots/plot_utils.py
Outdated
except ValueError as err: | ||
raise ValueError("Verify coords keys. {}".format(err)) | ||
|
||
except KeyError as err: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...are these right? A KeyError
gets thrown for bad .values
and a ValueError
for bad keys
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not. I'll verify. Thanks for double checking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So yes,
Having a wrong key in coords dictionary raises a ValueError in xarray, and having a value in the coords dictionary raises a KeyError.
I wrote tests but just double checked by hand as well
Ill make the exceptions more clear
There's a gist below of both xarray defined ValueError and KeyError, and also two Options at the bottom to consider for arviz Let me know which one you guys like, (or any other suggestions to try) and I'll implement throughout the library. https://gist.github.com/canyon289/cbd3744a36d8198c6d4ac796eae60e4b |
Could we have suggestion where to go to see correct coord/dim format? For new users these things can be complicated. Our docs + xarray docs? |
I agree it is complicated! There's two separate issues, one is the format, but the other is just trying to select something that doesn't exist. Maybe we should have three exceptions?
|
That does sound reasonable. I think if we add any links, they should be on their own lines (or something users can click): See. |
Turns out there's four paths to three types of exceptions all shown in the gist. I took the strategy of letting xarray raise the exception before handling it for two reasons
https://gist.github.com/canyon289/51281affde8e8a8891d503e45dea0920 In regards to @ahartikainen's comment, I do agree we need an "xarray 101" type documentation. For this PR though would you guys be ok with merging this (once I update all plots and it looks ok) and I'll follow up with the documentation, and link in exception, in another PR? |
Looks good to me - is this all the places |
LGTM |
36738df
to
93d4f7f
Compare
Updated to all the places with **sel. I'll merge if tests pasts. Thank you |
93d4f7f
to
8b50ca6
Compare
8b50ca6
to
e8c693c
Compare
Fix for #213
If you guys like how this looks I'll change all the other plots to this pattern. Did a couple as an example