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

Fix RGB imshow with X or Y dim of size one #1967

Merged
merged 1 commit into from
Mar 8, 2018
Merged

Conversation

Zac-HD
Copy link
Contributor

@Zac-HD Zac-HD commented Mar 6, 2018

Not much more to say, really. Thanks to @fmaussion for pinging me - definitely faster to track down when you know the code!

@fmaussion
Copy link
Member

Wow that was fast, thanks!

Would it be possible to add the test to the Common2dMixin class instead? (exmaple here)

@Zac-HD
Copy link
Contributor Author

Zac-HD commented Mar 6, 2018

No problem - I had an hour free and no open pulls waiting on me, so the timing was good.

As a regression test it's specific to imshow, so I'm not sure what you'd want here (or whether it would work at all on the 2d mixin). More details please?

@fmaussion
Copy link
Member

As a regression test it's specific to imshow, so I'm not sure what you'd want here (or whether it would work at all on the 2d mixin). More details please?

Yes sorry: I think it should work same for all four 2d plot methods (we should have had a test for this)

@Zac-HD
Copy link
Contributor Author

Zac-HD commented Mar 7, 2018

Ah, I see what you mean but don't think we need any change or additional test.

There's a catch though - xarray.plot() is special, because it (and only it) squeezes the dimensions of the array before plotting it. Therefore, the following calls all produce the same plot:

xr.DataArray(np.arange(9).reshape((3,3))).plot()
xr.DataArray(np.arange(9).reshape((1,3,3))).plot()
xr.DataArray(np.arange(9).reshape((1,1,3,1,1,1,1,3,1,1))).plot()

My view is that the test you linked to is sufficient for the test you're asking for - imshow is a special case because it can accept 3D input for RGB plots.

TLDR - working as intended IMO, it's just that nobody reads the docs. Changing the API would avoid this but at cost of convenience which is the whole point of DataArray.plot().

@shoyer
Copy link
Member

shoyer commented Mar 7, 2018

@Zac-HD your link on squeezing goes back to this page :).

Possibly we should deprecate and/or remove the auto-squeezing behavior? That sort of magic can be pretty frustrating to debug.

@Zac-HD
Copy link
Contributor Author

Zac-HD commented Mar 7, 2018

Not any more 😉 - it links to the docs that "[xarray.plot.plot()] calls an xarray plotting function based on the dimensions of darray.squeeze()".

I'd actually like to keep the magic in plot() - the whole point of this is that we can do something with the data, not that we do anything in particular. For interactive use, this is valuable enough - in my view - to justify keeping an inconsistent API; particularly when there is an obvious non-magic version to use in a script. In any case, that's a topic for another pull.

@shoyer
Copy link
Member

shoyer commented Mar 8, 2018

OK, I'm going to merge this. Thanks @Zac-HD

@shoyer shoyer merged commit 9accae0 into pydata:master Mar 8, 2018
@Zac-HD Zac-HD deleted the rgb-maps branch March 9, 2018 01:49
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 this pull request may close these issues.

imshow should work with third dimension of len 1
3 participants