-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Plot nans #1782
Plot nans #1782
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.
Thanks, much easier than expected! I have a few comments, and thanks also for the mpl crossref
doc/whats-new.rst
Outdated
|
||
- Bug fixes in :py:meth:`DataArray.plot.imshow`: all-NaN arrays and arrays | ||
with size one in some dimension can now be plotted, which is good for | ||
exploring satellite imagery. |
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.
Add "(:issue:1780
)"
xarray/plot/plot.py
Outdated
try: | ||
xstep = (x[1] - x[0]) / 2.0 | ||
except IndexError: | ||
xstep = .1 |
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.
nit: I find xstep = .5
more intuitive here
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.
ignore my comment: it doesn't matter
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.
Either way, it would be good to add a comment here noting that the choice is arbitrary
xarray/tests/test_plot.py
Outdated
@@ -624,6 +624,13 @@ def test_plot_nans(self): | |||
clim2 = self.plotfunc(x2).get_clim() | |||
self.assertEqual(clim1, clim2) | |||
|
|||
def test_can_plot_all_nans(self): | |||
# regression test for issue #1780 | |||
DataArray(np.full((2, 2), np.nan)).plot.imshow() |
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.
Do this instead:
self.plotfunc(DataArray(np.full((2, 2), np.nan)))
Common2dMixin
will check that all 2d functions work consistently
xarray/tests/test_plot.py
Outdated
DataArray(np.full((2, 2), np.nan)).plot.imshow() | ||
|
||
def test_can_plot_axis_size_one(self): | ||
DataArray(np.ones((1, 1))).plot.imshow() |
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.
Here too
6702761
to
ed8b62b
Compare
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. I have a couple minor documentation suggestions.
doc/whats-new.rst
Outdated
- Bug fixes in :py:meth:`DataArray.plot.imshow`: all-NaN arrays and arrays | ||
with size one in some dimension can now be plotted, which is good for | ||
exploring satellite imagery. (:issue:`1780`) | ||
- Bug fixes in :py:meth:`DataArray.plot.imshow`: all-NaN arrays and arrays |
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 entry is now repeated twice :)
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.
Ah, rebasing! Fixed 😄
xarray/plot/plot.py
Outdated
try: | ||
xstep = (x[1] - x[0]) / 2.0 | ||
except IndexError: | ||
xstep = .1 |
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.
Either way, it would be good to add a comment here noting that the choice is arbitrary
xarray/plot/utils.py
Outdated
@@ -165,6 +165,10 @@ def _determine_cmap_params(plot_data, vmin=None, vmax=None, cmap=None, | |||
|
|||
calc_data = np.ravel(plot_data[~pd.isnull(plot_data)]) | |||
|
|||
# Handle all-NaN input data gracefully | |||
if calc_data.size == 0: | |||
calc_data = np.array(0.0) |
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 suppose the colorbar get the wrong data in this case, but it's still better than an error message?
Again, please comment that the value 0 is arbitrary.
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.
There is no data to give the colorbar in this case, so it uses Matplotlib defaults. Works beautifully for faceted plots of image timeseries though 😄
Thanks @Zac-HD ! |
No problem! Do you have any idea when this might be released? I can install from git in my own environment, but I probably can't convince GA to do that in their production environment 😄 |
There are a few other bugs that fell out of the v0.10 release. Once we fix those it would make sense issue another release soon... Maybe within a few weeks? |
git diff upstream/master **/*py | flake8 --diff
(remove if you did not edit any Python files)whats-new.rst
for all changesCC @fmaussion for review; @BexDunn for interest