-
-
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
Feature radial heatmap #2155
Feature radial heatmap #2155
Conversation
This is looking great already!
Looks like this is a bug in the implementation, I'll have a PR fixing this tomorrow.
I briefly investigated exposing
Looks like this is true for all CompositePlot classes. The problem lies with ColorbarPlot._init_glyph never getting called. Really
While this is true in general I don't think it's necessarily a huge problem for a radial heatmap because the heatmap is always a fixed size. Making space for the labels is still a problem though so I'll mull it over.
I agree this is probably necessary, would be nice if we could find a shorter name though. Some other to do items include (I'm happy to help or take on any or all of these):
Thanks for the contribution, the code looks very clean and I'd be very happy to see this included with a bit more work. |
holoviews/plotting/bokeh/raster.py
Outdated
|
||
|
||
@staticmethod | ||
def _extract_implicit_order(array): |
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 you can use holoviews.core.util.unique_array
or holoviews.core.util.unique_iterator
for this instead.
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 for the hint - no need to reimplement what is already there :-)
holoviews/plotting/bokeh/raster.py
Outdated
|
||
|
||
# get orders | ||
order_segment = self._extract_implicit_order(xvals) |
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'd actually just use aggregate.dimension_values(x, expanded=False)
and aggregate.dimension_values(y, expanded=False)
here instead.
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.
Agreed - definitely better.
I took the liberty of implementing the matplotlib version of this plot, which you can see here. If it's okay with you I'll push that to the PR tomorrow. Here's the output: The only change I made to the parameters was using the |
Looks good! Is it intentional that the annular wedges in the matplotlib version are polygonal rather than actual annuli? It's an odd effect even in the above plot, and I can't see how the above plot would work at all with fewer radial divisions. I.e., what happens when a radial section isn't convex (e.g. if one section covers more than 180 degrees), or even just if there are only two 180-degree sections? Also, it might be nice to have an option for having the axial spacing be constant in terms of area, so that the space occupied by Tuesday 31 is the same as that occupied by Sunday 15 (which requires the ring spacing to be nonlinear). |
No but I really don't know of a non-painful way to draw them correctly, all I can think of doing to handle the case of a low number of bins is to upsample the data by a scalar factor along that xaxis to ensure a minimum number of bins. Also worth noting that only the matplotlib version suffers from this issue, @mansenfranzen's bokeh implementation appropriately draws annular wedges. |
Right; I updated my comment to refer just to the mpl version after seeing the beautiful wedges on the Bokeh version. Do the wedges illustrated here not work? |
Should be straightforward to do, but I'm unsure if that would be very interpretable in many cases. |
I'm not sure how well it would work in practice, but currently the default equal-spacing approach is highly misleading about the overall distribution of values. That's a common problem with this type of plot... |
Possibly, I suppose the number of wedges will never be huge, but I can see this being slow. Also haven't yet found any examples with polar axes but it's worth a shot. |
Sure, go ahead - great to see you've implemented the MPL version so quickly :-).
I'm happy to do so: do you have an example test module of an already implemented bokeh plot class?
Sure, I will add a reference documentation later this week.
Sounds like an interesting feature to implement. Would be great to see the effect of this in practice. Hope to add this soon, too. And thx for the typo fixes ;-). |
It actually turned out to be fairly easy, because I could mostly reuse your existing code and because matplotlib actually handles polar axes so I didn't have to implement custom labelling like you did. |
Sounds like a great visualization example. I think the viz could be even more striking if we restrict temperatures to be from one country/region (Germany or UK) in order reveal seasonal effects between winter and summer months, too. |
Looks great! I'd vote for that even if it's slower, because it's a lot clearer and should behave well for any wedge size. It would be great to have a weekly example (seems like one could plot ridership numbers for the nyc_taxi data) where weekends stand out clearly, plus a monthly example like the temperature data, hopefully with clear cyclical variations in both cases to make it clear why to use this type of plot. |
The plotting order bug should be fixed by #2169, which I just merged. |
56716a1
to
8671924
Compare
@philippjfr Thanks for fixing the plotting order bug. It works as expected now. I did a rebase on the current feature branch to rewrite the commit history and to include your fix. I've simplified and refactored the existing code for better readability using more holoviews builtins as suggested by @philippjfr . In addition, I added support for annular tick labels. I dropped the Reference exampleI also added a reference example with the two examples of global surface temperatures and the NYC taxi dataset: Absolute global temperatures since 1750NYC Taxi pickup counts for 2016For more detail on the plot examples, take a look at the reference examples. The NYC example is pretty good :-). I haven't finished the reference example, yet. But please feel free to modify, comment or improve my English :-). I added two datasets to the examples/assets directory which are used in the reference example. Both datasets are freely available. I modified them for easier usage. Creation and source can be found here. I'm not completely sure on how to handle sample datasets and how this is supposed to be in holoviews. I've thought about adding the datasets to bokeh.sampledata as bokeh already has an existing sample data setup. Still open
|
Great!
Perfect!
Both look great, particularly the NYC taxi one. I think we can drop the temperature idea. I'm not sure about the dataset yet. I think we do need a mechanism to download larger datasets as we do want a wide range of examples.
I'm planning on moving all of these into each respective plotting backend but for now https://github.com/ioam/holoviews/blob/master/tests/testbokehgraphs.py probably provides the best example.
Sounds good, maybe we can also find shorter names for those parameters.
I'd be happy to merge this without it and fix it up in #2171, since that will probably require a fair bit of discussion. |
Here is the current update on the radial heatmaps: Additions / Modifications:
Open:
Issues:
|
Great, I'll try to review this soon.
Sounds good.
Sounds good! I agree tickers don't make much sense here and I don't think they are used much. We do support supplying ticks in the form
Sounds good!
I'll handle these.
Yes, this is a definite problem I've also come across in other context. We'll open an issue to allow them to be styled separately once this is supported. |
@jbednar @jlstevens Any ideas for a shorter name than |
@mansenfranzen First off, these plots are beautiful and thanks for contributing!
Good question, I'll have to think about it. Ideally, there is a unique name so it sounds like an element with its own identity but I can only think of compound names right now. edit: Or |
What about |
explaination for choosen plot options.
drawn. Fix bug in `get_extents` which ignored the `max_radius` plot option. Fix bug in `get_default_mapping` which also ignored the impact of `max_radius`. Refactor and simplify `compute_tick_mapping`. Remove extra offset from x and y coordinates for xticks as it is not required.
convention as the bokeh plot.
c9b0930
to
bde9d08
Compare
Looks good to me and @mansenfranzen, thank you very much for your contribution! |
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 is not meant to be finished but is rather a work in progress for discussion purposes.
I implemented a first version of a radial heatmap which still has several limitations:
Issues
Plotting order: In order to enhance readability, one can define separation lines to distinguish groups of annular wedges more clearly via plot option
separate_nth_segment
. However, sometimes they show up, sometimes they don't. I guess this is due to the z-order in which glyphs are plotted. I added the_draw_order
class attribute without any effect.Match aspect: Coordinates of annular wedges, lines and text do not result in corresponding alignments (same coordinates differ in visual field). This is a known issue in bokeh, see here #7218. One workaround I've found is to completely remove any range and axis definition from a bokeh figure and set
match_aspect
toTrue
in order to get the correct aspect ratio (however one has to overwrite many inherited methods...). Another way is to manually adjust the width and height to match the aspect ratio but this depends on toolbar location, colobar and other elements and hence is not really meaningful. Therefore, I guess we have to wait until a bokeh site of fix.Colobar does not show up if selected
Input key dimension differ to output dimension: The key dimensions of the input data are categorical. However, the plot is based on numerical values a in interval scaled coordinate system. As far as I can understand the class design, holoviews expects input key dimension to map on the actual x and y ranges. I've thought about creating a custom
holoviews.Operation
to convert the categorical input into numerical radii and radiants just likehistogram
.Still missing:
Open question:
RadialHeatMap
class: As discussed in Add Radial Heatmap #2139, I decided to create its own element class representation instead of using theHeatMap
element class because the radial heatmaps also require custom layout options which are very different from the rectular heatmap requirements. WIthout the extra class, I wasn't able to set the visualization defaults in the bokeh/init.py properly.Overall, the plain example works in examples/gallery/demos/bokeh/radial_heatmap.ipynb.
I'm very happy for every feedback.