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

QuadMesh transposed? #217

Closed
jbednar opened this issue Jul 15, 2015 · 13 comments
Closed

QuadMesh transposed? #217

jbednar opened this issue Jul 15, 2015 · 13 comments
Assignees
Labels
Milestone

Comments

@jbednar
Copy link
Member

jbednar commented Jul 15, 2015

Unless I've gotten confused, QuadMesh seems to be transposing its data, compared to Points:

import numpy as np
import holoviews as hv
%load_ext holoviews.ipython
%opts Points (s=300)
print hv.__version__pts=np.array([[2.1,5.6],[0.8,-3.1],[0.1,4],[0.4,3]])
hist=np.histogram2d(pts[:,0], pts[:,1], bins=20)
hv.Points(pts) + hv.QuadMesh((hist[1],hist[2],hist[0]))

1.3.2-29-gca8818e

image

Also, I would guess that np.histogram2d would be a common source of input for QuadMesh; is there a more convenient way to call QuadMesh on such data?

Conversely, since this is all regularly sampled anyway, is there a good way to build an Image from np.histogram2d, rather than using QuadMesh?

@jbednar
Copy link
Member Author

jbednar commented Jul 16, 2015

It works if I use hist[0].T instead of hist[0], but between the QuadMesh documentation and the histogram2d documentation, I can't figure out what format the output and input of these two are supposed to be in. It may be working as designed, but if so (a) needs more documentation, and (b) seems unnecessarily difficult to use these two items together (histogram2d and QuadMesh).

@jlstevens
Copy link
Contributor

I believe Points and Image overlay as you would expect because they share the same convention for the origin (i.e matching plt.imshow). As QuadMesh is designed to follow the behavior of plt.pcolor (in order to help matplotlib users that are used to either pcolor or pcolormesh) we have inherited the corresponding convention which does not match the behavior of plt.imshow.

Hopefully that explains why this behavior is actually expected and why you need to transpose the data first. Essentially, we've decided to inherit the (inconsistent!) conventions regarding the origin from matplotlib (which in turn, probably came from Matlab).

The inconsistent conventions are annoying but the main reason we introduced QuadMesh is to help make life easier for people using plt.pcolor. For everyone else, I would expectRaster or Image to be used by default (at least if the spatial sampling is already regular).

Edit: I agree with your point about improving the documentation and maybe showing the relationship between histogram2d and QuadMesh more clearly.

@philippjfr
Copy link
Member

The differing conventions are pretty annoying and I'm not sure what best to do about it as it does follow the conventions some people are used to. For example I imagine Bas might actually be happy to finally have an Element that follows the convention he's used to.

In terms of histogram2d, I would suggest that we should have a histogram2d operation, just like the histogram operation we have currently, which will avoid having to call and construct these objects directly. Whether to add a .hist2d method on the Elements I'm not so clear about but it might turn out to be useful enough to consider. Similarly we could also consider making the contours operation into a method on Raster types.

Adding more documentation is of course always a good idea.

Edit: Just confirmed that the behavior is what you'd expect.

@jbednar
Copy link
Member Author

jbednar commented Jul 16, 2015

A histogram2d operation does seem useful. If anyone does it, make sure to allow different numbers of bins in the two directions; numpy only seems to support the same number in both, which isn't necessarily meaningful if the two dimensions are very different.

I'm still not sure how it is that the data needs .T, but maybe rather than explaining it here, you can explain it in the docstring. It still seems like an error to me!

In any case, having one Element have a different convention to match some external convention doesn't seem like an improvement, since it will make it harder to switch between elements and to remember the conventions of each one.

@jlstevens
Copy link
Contributor

In any case, having one Element have a different convention to match some external convention
doesn't seem like an improvement, since it will make it harder to switch between elements and to
remember the conventions of each one.

I do agree but do you think it is worth breaking the various conventions used by matplotlib? Should we force people like Bas to transpose their data even if they are used to just using plt.pcolor directly?

@jbednar
Copy link
Member Author

jbednar commented Jul 22, 2015

I'm not sure how we could ever match the various conventions used by matplotlib, and doing so would only make sense if we did it consistently, which requires that matplotlib be consistent. In the Coordinates tutorial we state explicitly that we consistently interpret arrays the same way that they are printed on the terminal (which is also the mathematical convention for writing arrays), yet QuadGrid currently fails to match that (unlike all our other classes). I vote that QuadGrid, since it is brand new and yet to appear in a release, be fixed to match the convention of all other Elements, but that if necessary we provide a wrapper function whose explicit purpose is to match the conventions of matplotlib. E.g. hv.pcolor(data) would then act like mpl.pcolor, if it's particularly helpful to match matplotlib in that particular case.

@philippjfr philippjfr self-assigned this Sep 16, 2015
@philippjfr philippjfr added this to the v1.4.0 milestone Sep 16, 2015
@philippjfr philippjfr mentioned this issue Nov 10, 2015
42 tasks
@philippjfr
Copy link
Member

Okay, I just checked and it seems like the situation is even more confusing than we (or at least I) thought. The pcolormesh call expects the data to be both transposed and flipped compared to our Images. I say we match the output np.hist2d, which means your example would work without transposing the data. This is still flipped along the y-axis when compared to our Images but anything else would require you to also fiddle with the x- and y-coordinates (reverse them maybe) when dealing constructing a QuadMesh from hist2d output.

@jbednar
Copy link
Member Author

jbednar commented Nov 17, 2015

Ok by me; just looking at this thread again gives me a headache. :-)

@philippjfr
Copy link
Member

Ugh, same here. Turns out I was confused again and the current format does make sense, the hist2d output has shape (x, y), while all our rasters are (y, x). So transposing it first is the correct thing to do. It is however then still flipped compared to our Images but I reckon requiring this np.flipud(hist[0].T) when constructing the QuadMesh from hist2d output seems a bit much, especially since that's the primary use case.

TL;DR I'm in favour of keeping the current behavior.

@jbednar
Copy link
Member Author

jbednar commented Nov 17, 2015

Ok, but it seems to me like the current behavior is stuck somewhere in the middle, being neither convenient for the primary use case nor matching the rest of HoloViews.

In any case, please add a hist2d example to the QuadMesh section of the online notebook, mentioning that the data needs to be swapped as hinted above and that the convention also differs from the rest of HoloViews.

@jlstevens
Copy link
Contributor

Even if the conventions don't make sense (they probably don't) I would rather follow someone else's conventions rather than make up our own. There are many people who are used to matplotlib and matlab and my understanding is that we are just following what matplotlib does.

Coming up with a consistent system would be a great idea if there weren't so many people used to the inconsistent one. In other words, I don't think we can make everyone happy anyway so I would play safe and blame someone else for the mess. ;-p

@philippjfr
Copy link
Member

Okay, so we'll agree to keep the the current convention, it's the format Bas was expecting when he first asked for an Element like this and it matches the convention people will be used to from matplotlib. I'll add the example and documentation now and close the issue.

Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants