-
Notifications
You must be signed in to change notification settings - Fork 45
Support sampling face-centered data onto a GeoAxes
#1271
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
Conversation
imshow function for plotting with Matplotlib
|
Let me know if you have any feedback on this. I'm curious whether you think |
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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.
I'm a little worried that the API here might be confusing and not particularly intuitive for users familiar with conventions in the broader scientific Python ecosystem for a couple of reasons.
The first being that it deviates from other high level plotting APIs in the scientific Python ecosystem that use .plot() and .hvplot(). This is definitely beyond the scope of this PR, but adds some potential for confusion and complexity here.
And the second that imshow() here is pretty different from how it's used in Xarray and Matplotlib (e.g. there's a good bit going on with the resampling, Cartopy is used by default, etc.). Maybe a new name would be appropriate?
That said, I don't know that I have a great suggestion for what to do here. I suspect that something chained onto the .plot() accessor would be more intuitive, but that seems a bit complicated and potentially confusing given what's already present. Maybe implementing something under uxarray.plot.mpl.plot() for a high level Matplotlib plotting interface could work?
Edited to add: I should also say it's really exciting to see this and I think folks will very much appreciate having better support for Matplotlib and the performance improvements.
|
Should be good for another set of reviews. I've made some reccomended changes after discussing with @erogluorhan , notably changing the signature to be Any feedback on the update notebook would be much appriciated, especially from @brianpm |
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.
Thanks for putting this together! I like it overall; just see a few comments below please.
|
I've added a "Conversion Methods" section in the API which can house our https://uxarray--1271.org.readthedocs.build/en/1271/api.html#conversion-methods |
This sounds great and sure will help users find them much easier! |
erogluorhan
left a comment
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.
Looks great; approving with a couple minor suggestions.
Co-authored-by: Orhan Eroglu <32553057+erogluorhan@users.noreply.github.com>
Co-authored-by: Orhan Eroglu <32553057+erogluorhan@users.noreply.github.com>
kafitzgerald
left a comment
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.
Looks good to me!

Closes #1268, #366
Overview
UxDataArray.to_raster()method, which is used to sample an unstructured grid onto a provided GeoAxesimshow,contourandcontourfExample