-
-
Notifications
You must be signed in to change notification settings - Fork 403
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
GridInterface bin support for Histogram and QuadMesh #2160
Conversation
1fedff8
to
dc1ca4b
Compare
Here is an overview of the new functionality. |
Nice! |
@jbednar One question I have is how we can properly support datashading a QuadMesh, currently I see two options:
Both don't seem quite right so the real solution is getting actual support for quadmeshes into datashader. |
What about slicing every bin in half and rendering it as a TriMesh? |
9270153
to
73696bb
Compare
5b298b3
to
793ac32
Compare
276dba6
to
b41765f
Compare
c0b09b2
to
4e6b5ce
Compare
4e6b5ce
to
36a2ec4
Compare
@jbednar @jlstevens Requesting review. |
a68631d
to
7aabc20
Compare
"Qz = np.sin(Y) + np.sin(X)\n", | ||
"Z = np.sqrt(X**2 + Y**2)\n", | ||
"\n", | ||
"print(Qx.shape, Qz.shape, Z.shape)\n", |
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.
Stray print? If not it needs to print a proper message.
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.
Sure, just meant to show how the data differs between unevenly sampled and irregularly sampled QuadMeshes.
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.
Ok. Then I would draw attention to that more explicitly including a proper print message, maybe in a separate cell.
t3s = (js+1)*(s0+1)+t1%s0 | ||
t4s = t2s | ||
t5s = t3s | ||
t6s = t3s+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.
This is pretty unreadable and the variable names are uninformative.
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 this is doing some self-contained computation, it would be better as a utility which can at least have a docstring explaining what this is supposed to be doing.
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.
Sure, a utility is fine.
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.
End up with all kinds of circular import issues if I try to move it to element.util
, I've added various comments which should clarify.
holoviews/element/raster.py
Outdated
|
||
Note: Deprecate as part of 2.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.
Is this being tested via the current notebook tests?
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.
It was, but once tests are rebuilt just once it won't be.
raise error('Key dimension values and value array %s ' | ||
'shapes do not match. Expected shape %s, ' | ||
'actual shape: %s' % (vdim, expected[::-1], shape), cls) | ||
return data, {'kdims':kdims, 'vdims':vdims}, {} | ||
|
||
|
||
@classmethod | ||
def irregular(cls, dataset, dim): |
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.
You've used the word irregular
a lot. I am wondering if uneven
would have been clearer though it probably isn't worth changing now.
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.
irregular != uneven, they are different concepts.
if edges and not isedges: | ||
data = cls._infer_interval_breaks(data) | ||
elif not edges and isedges: | ||
data = np.convolve(data, [0.5, 0.5], 'valid') |
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.
Why is there a convolution 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.
Easiest way to compute a rolling mean, i.e. edges -> bin centers.
Looks fine and happy to merge once you've addressed my questions above. |
After discussing 'uneven' versus 'irregular', I think an irregular mesh example where the sample positions have a small (but obvious) random jitter would make the meaning of 'irregular' a lot clearer. |
Tests have passed. Ready to merge? |
Yes, let's merge, should get as much testing and exercising as possible before the next release. |
Merged! |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This PR adds support for binned coordinates to the GridInterface addressing #547. This is one of the final conversions of Elements to a Dataset type.
__setstate__
for pickle compatibilitySeveral nice to haves, which don't necessarily have to hold up merging this PR:
I've now managed to also support irregular meshes, which are a commonly requested feature particularly for GeoViews. Support for datashading quadmeshes will come in a later PR (probably after TriMesh is merged). Here are some irregular meshes:
And all the usual machinery also works to explore high-dimensional datasets, e.g. here is an example of a multi-dimensional irregularly gridded dataset (requested in holoviz/geoviews#57 and holoviz/geoviews#73).