-
-
Notifications
You must be signed in to change notification settings - Fork 404
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
Make HeatMap more general #849
Conversation
9ee8034
to
6d2eca3
Compare
6d2eca3
to
097680b
Compare
@jlstevens The PR is now ready for review, I'll rebuild the test data shortly. The test data will have to be updated because |
Still need to add thorough unit tests for the aggregation. I'm also now wondering whether I should add a warning when you pass non-aggregated data to a HeatMap, e.g. there are two different values for index ('A', 'a') in the heatmap, it would just silently ignore the second value. Really it should warn you that you should aggregate your data using some function (mean, max, min) before passing it to the HeatMap. |
Also deprecates support for unpickling the original |
|
||
|
||
def get_2d_aggregate(obj): | ||
""" |
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.
Perhaps this would be better expressed as an operation? Then maybe it could have a minimal docstring example in the class docstring?
for k1, k2 in product(keys1, keys2)}) | ||
return dense_map.dframe() | ||
return super(HeatMap, self).dframe() | ||
self.gridded = get_2d_aggregate(self) |
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.
Nice to see how much HeatMap
has been simplified!
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.
That said, it isn't immediately obvious that gridded
is now a Dataset
. Not sure I am necessarily recommending changing the name as gridded_dataset
is awkward...
@@ -383,85 +381,18 @@ class HeatMap(Dataset, Element2D): | |||
|
|||
vdims = param.List(default=[Dimension('z')]) | |||
|
|||
def __init__(self, data, extents=None, **params): | |||
depth = 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.
I might have forgotten...what is this depth
class attribute?
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 think this may be wrong now, will have to look into it.
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.
Wasn't needed at all in the end, removed it.
@@ -130,26 +136,31 @@ class HeatmapPlot(ColorbarPlot): | |||
def _axes_props(self, plots, subplots, element, ranges): | |||
dims = element.dimensions() | |||
labels = self._get_axis_labels(dims) | |||
xvals, yvals = [element.dimension_values(i, False) | |||
agg = element.gridded | |||
xvals, yvals = [unique_array(agg.dimension_values(i, False)) |
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 thought gridded Datasets
have the 1D coordinate arrays available. Is the uniqueness being applied over the 2D set of samples or the 1D sequence?
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.
Yes, good point, no longer any need for the unique_array here.
for i in range(2)] | ||
data = {x: xvals, y: yvals, z: zvals} | ||
|
||
if 'hover' in self.tools+self.default_tools: | ||
for vdim in element.vdims[1:]: | ||
data[vdim.name] = ['' if is_nan(v) else v |
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.
Wondering if an empty string really suggests NaN. 'NaN' would be explicit but might look noisy.
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.
Good point, I'm now using masked arrays to represent the data, in matplotlib the NaNs are therefore represented by -
, which might be better.
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.
Yes, I think -
might be a good compromise.
shape = data.shape | ||
cmap_name = style.pop('cmap', None) | ||
cmap = copy.copy(plt.cm.get_cmap('gray' if cmap_name is None else cmap_name)) | ||
cmap.set_bad('w', 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.
Might want to make this a plot option at some point instead of hard coding 'w'.
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.
Again good point, indeed we already expose this via clipping_colors
, should hook that in 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.
I also find it curious that you are using copy.copy
on a colormap - which suggests you are mutating it. I guess set_bad
must have side-effects which explains the copying...
Ok, I've made my comments for now (and you have already replied to most of them). The biggest suggestion is that |
Yes, |
@jlstevens Once tests are passing this is ready for a second review and then merge. |
@jlstevens Tests now passing on the PR build. |
@@ -647,6 +647,30 @@ def walk_depth_first(name): | |||
(names_by_level.get(i, None) | |||
for i in itertools.count()))) | |||
|
|||
|
|||
def is_cyclic(graph): | |||
"""Return True if the directed graph g has a cycle.""" |
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.
What is the representation of the graph? A list of edges as tuples? Would be good to mention in the docstring.
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 guessing the representation is similar as in one_to_one
...even so, probably worth mentioning..
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.
Right, all three methods here (sort_topologically
, cyclical
and one_to_one
) use the same representation, which is mapping between nodes and edges, will add the docstring.
return np.NaN | ||
|
||
|
||
class categorical_aggregate2d(ElementOperation): |
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! I was just wondering if you want to keep this class in util
or move it to operation.element
?
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's imported there but can't be moved, cyclical imports again.
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, having it available for operation.element
is fine.
Generates a categorical 2D aggregate by inserting NaNs at all | ||
cross-product locations that do not already have a value assigned. | ||
Returns a 2D gridded Dataset object. | ||
""" |
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.
Quite a long method...if you see chunks that could be split up into helper methods, that might be sensible. Up to you though!
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.
Happy to split it up.
I made three more comments for you to reply to. Otherwise looks good and I expect this will be merged very soon! |
Latest comments addressed, ready to merge when tests pass. |
Tests passed. Merging! |
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. |
The HeatMap Element has gone through a lot of redesign, initially being defined as a Raster type and then allowing columnar data. This PR refactors HeatMap to improve the simplify aggregation step and allow any number of value dimensions which can appear as hover information in bokeh. Still a WIP and I need to ensure the code is backward compatible (it should be).