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

Feature radial heatmap #2155

Merged
merged 38 commits into from
Mar 19, 2018
Merged

Conversation

mansenfranzen
Copy link
Contributor

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 to True 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 like histogram.

Still missing:

  • Add labels for second key dimension: At the moment, labels are only available for the first key dimension.

Open question:

  • Separate RadialHeatMap class: As discussed in Add Radial Heatmap #2139, I decided to create its own element class representation instead of using the HeatMap 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.

@philippjfr
Copy link
Member

philippjfr commented Nov 27, 2017

This is looking great already!

Plotting order

Looks like this is a bug in the implementation, I'll have a PR fixing this tomorrow.

Match aspect

I briefly investigated exposing match_aspect on all our plot classes but didn't really have much luck. I'll try to look into it again, so hopefully we can use it here.

Colobar does not show up if selected

Looks like this is true for all CompositePlot classes. The problem lies with ColorbarPlot._init_glyph never getting called. Really ColorbarPlot should be a mixin class, looks like I'll have to do some refactoring here.

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.

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.

Separate RadialHeatMap class: As discussed in #2139, I decided to create its own element class representation instead of using the HeatMap 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.

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):

  • Add reference material in examples/reference/element/ (the current demo is a good starting point)
  • Add a few plot tests basically testing whether the datasources and glyphs are initialized correctly.
  • Add an equivalent implementation for the matplotlib backend.

Thanks for the contribution, the code looks very clean and I'd be very happy to see this included with a bit more work.



@staticmethod
def _extract_implicit_order(array):
Copy link
Member

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.

Copy link
Contributor Author

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 :-)

@philippjfr philippjfr added the type: feature A major new feature label Nov 27, 2017
@philippjfr philippjfr added this to the v1.10 milestone Nov 27, 2017


# get orders
order_segment = self._extract_implicit_order(xvals)
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed - definitely better.

@philippjfr
Copy link
Member

philippjfr commented Nov 27, 2017

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:

image

The only change I made to the parameters was using the xticks and yticks parameters to control the number of ticks.

@jbednar
Copy link
Member

jbednar commented Nov 27, 2017

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).

@philippjfr
Copy link
Member

philippjfr commented Nov 27, 2017

Is it intentional that the annular wedges are polygonal rather than actual annuli?

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.

@jbednar
Copy link
Member

jbednar commented Nov 27, 2017

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?

@philippjfr
Copy link
Member

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).

Should be straightforward to do, but I'm unsure if that would be very interpretable in many cases.

@jbednar
Copy link
Member

jbednar commented Nov 27, 2017

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...

@philippjfr
Copy link
Member

philippjfr commented Nov 27, 2017

Do the wedges illustrated here not work?

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.

@philippjfr
Copy link
Member

Here it is using matplotlib Wedges:

image

@mansenfranzen
Copy link
Contributor Author

@philippjfr

If it's okay with you I'll push that to the PR tomorrow.

Sure, go ahead - great to see you've implemented the MPL version so quickly :-).

Add a few plot tests basically testing whether the datasources and glyphs are initialized correctly.

I'm happy to do so: do you have an example test module of an already implemented bokeh plot class?

Add reference material in examples/reference/element/ (the current demo is a good starting point)

Sure, I will add a reference documentation later this week.


@jbednar

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).

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 ;-).

@philippjfr
Copy link
Member

Sure, go ahead - great to see you've implemented the MPL version so quickly :-).

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.

@philippjfr
Copy link
Member

Just some quick thoughts on demos and reference material, I think if we could simplify the current example a little bit, it's fine as the reference example. For the gallery demo I would like to use global temperature anomaly data unless you can think of something better, something like this:

image

@mansenfranzen
Copy link
Contributor Author

Just some quick thoughts on demos and reference material, I think if we could simplify the current example a little bit, it's fine as the reference example. For the gallery demo I would like to use global temperature anomaly data unless you can think of something better, something like this:

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.

@jbednar
Copy link
Member

jbednar commented Nov 27, 2017

Here it is using matplotlib Wedges

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.

@philippjfr
Copy link
Member

The plotting order bug should be fixed by #2169, which I just merged.

@mansenfranzen mansenfranzen force-pushed the feature_radial_heatmap branch 2 times, most recently from 56716a1 to 8671924 Compare December 3, 2017 13:29
@mansenfranzen
Copy link
Contributor Author

@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 show_nth_segement plot option in favour of xticks. Segment labels can now be set natively via xticks while annular labels can be defined via yticks.

Reference example

I also added a reference example with the two examples of global surface temperatures and the NYC taxi dataset:

Absolute global temperatures since 1750

globaltemperatures

NYC Taxi pickup counts for 2016

@jbednar
nyc_taxi_2016

For 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

  • Tests: Can you please provide an example of other plot classes having tests already? I need some guidance here.
  • Annular separation lines: Basically the same as the segment separation lines.
  • Match aspect workaround

@philippjfr
Copy link
Member

philippjfr commented Dec 5, 2017

@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.

Great!

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 show_nth_segement plot option in favour of xticks. Segment labels can now be set natively via xticks while annular labels can be defined via yticks.

Perfect!

I also added a reference example with the two examples of global surface temperatures and the NYC taxi dataset:

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.

Tests: Can you please provide an example of other plot classes having tests already? I need some guidance here.

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.

Annular separation lines: Basically the same as the segment separation lines.

Sounds good, maybe we can also find shorter names for those parameters.

Match aspect workaround

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.

@mansenfranzen
Copy link
Contributor Author

mansenfranzen commented Dec 7, 2017

Here is the current update on the radial heatmaps:

Additions / Modifications:

  1. The reference example now focuses on the NYC example providing some modification using several plot options to enhance the plot. It still contains the global temperature example at the end. Feel free to remove it if you want but it also provides some additional information about plot options.
  2. The seperate_nth_segment was renamed to xmarks in analogy to xticks. Accordingly, ymarks was also added to add separation lines for annulars. The y/xmarks allow integers, lists, tuples and functions to support different requirements.
  3. x/yticks now also support python functions to define which ticks should be shown or not. A custom bokeh ticker does not make sense here because there is not real bokeh axis object because we have to fake the polar axis.
  4. Several tests were added for the bokeh plot.
  5. A new yticks_angle plot option was added to define the angle along which the yticks are drawn.

Open:

  1. The MPL plot is missing the ymarks and yticks_angle plot options but this shouldn't be a great deal.
  2. The MPL plot is also lacking tests.
  3. The MPL plot does not have a reference example yet.

Issues:

  1. Text glyphs are used to create the xticks and yticks. It would be great to modify their visual text properties like size and color independently. However, it seems to as if this is not possible at the moment because _style_groups does not allow to specify the same glyph to multiple style groups. I can only change the visual property of text glyphs in general but not for a custom group of text glyphs.

@philippjfr
Copy link
Member

The reference example now focuses on the NYC example providing some modification using several plot options to enhance the plot. It still contains the global temperature example at the end. Feel free to remove it if you want but it also provides some additional information about plot options.

Great, I'll try to review this soon.

The seperate_nth_segment was renamed to xmarks in analogy to xticks. Accordingly, ymarks was also added to add separation lines for annulars. The y/xmarks allow integers, lists, tuples and functions to support different requirements.

Sounds good.

x/yticks now also support python functions to define which ticks should be shown or not. A custom bokeh ticker does not make sense here because there is not real bokeh axis object because we have to fake the polar axis.

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 [position1, position2, ...] and [(position1, label), (position2, label), ...] but this does not work for categorical axes, so I wouldn't really expect to support it here.

Several tests were added for the bokeh plot.

Sounds good!

The MPL plot is missing the ymarks and yticks_angle plot options but this shouldn't be a great deal.
The MPL plot is also lacking tests.
The MPL plot does not have a reference example yet.

I'll handle these.

Text glyphs are used to create the xticks and yticks. It would be great to modify their visual text properties like size and color independently. However, it seems to as if this is not possible at the moment because _style_groups does not allow to specify the same glyph to multiple style groups. I can only change the visual property of text glyphs in general but not for a custom group of text glyphs.

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.

@philippjfr
Copy link
Member

@jbednar @jlstevens Any ideas for a shorter name than RadialHeatMap?

@jlstevens
Copy link
Contributor

jlstevens commented Dec 12, 2017

@mansenfranzen First off, these plots are beautiful and thanks for contributing!

Any ideas for a shorter name than RadialHeatMap?

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. AnnularBins? A little bit of research might help come up with a better name...

edit: Or CyclicBins? Though Radial is a better prefix so RadialBins maybe?

@jbednar
Copy link
Member

jbednar commented Dec 12, 2017

What about HeatMap? Seems to me that whether it's radial or not is simply a plot option, and from a user perspective there's no semantic difference between a radial and a non-radial heat map. I know that would take some fiddling with the code, but it seems like the implementation here has gradually become more like that of the regular HeatMap, so that they are no longer so very far apart in terms of their parameters and behavior.

mansenfranzen and others added 25 commits March 17, 2018 13:11
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.
@philippjfr philippjfr force-pushed the feature_radial_heatmap branch from c9b0930 to bde9d08 Compare March 17, 2018 13:12
@jlstevens
Copy link
Contributor

Looks good to me and @mansenfranzen, thank you very much for your contribution!

@jlstevens jlstevens merged commit 6a127d0 into holoviz:master Mar 19, 2018
Copy link

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.

@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
type: feature A major new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants