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

Ternary contour plot #1413

Merged
merged 12 commits into from
Feb 1, 2019
Merged

Ternary contour plot #1413

merged 12 commits into from
Feb 1, 2019

Conversation

emmanuelle
Copy link
Contributor

@emmanuelle emmanuelle commented Jan 27, 2019

This is a work in progress version of contour plots in ternary diagrams. I'm opening the PR even if it's too early to merge or do a thorough feedback, but I'd appreciate some quick feedback to know whether this is a good direction to go.

TODO
[x] Docstrings for helper functions.
[x] Improve coding style of helper functions in places.
[x] Decide whether other arguments (kw) need to be exposed in create_ternaryplot
[x] Write more tests, in particular when errors are raised.

Other questions : where in the doc can I write a tutorial about this new function?

@emmanuelle
Copy link
Contributor Author

@empet @jackparmer

@jackparmer
Copy link
Contributor

Other questions : where in the doc can I write a tutorial about this new function?

https://github.com/plotly/documentation/blob/source-design-merge/_posts/python/README.md

@Michael Babyn or @priyatharsan (Slack handles) can help you with contributing questions here. The pages that we want to replace/revamp are:

https://plot.ly/python/ternary-contour/
https://plot.ly/python/ternary-scatter-contour/

@jackparmer
Copy link
Contributor

jackparmer commented Jan 27, 2019

@nicolaskruchten can we please include some or all of these example datasets in px:
https://github.com/nicholasehamilton/ggtern/tree/master/data
@empet used some of these recently in her ternary plots: https://plot.ly/~empet#/
Wonder if there are other "classic" datasets for oil & gas and financial services that we should be including in px.


# Annotations for ticklabels on side 0
annotations.extend([dict(showarrow=False,
text=f'{ticklabel[j]}',
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately we can't use f-strings in released code yet because we still support Python 2.7 and 3.5. (Note that we do use them in the code generation logic, but this isn't part of the released bundle).

@jonmmease
Copy link
Contributor

This looks really cool @emmanuelle! I'll look through the code in more detail soon but the overall concept and structure look fine, so feel free to keep on working through your TODO list.

ALso, as you make more examples, please add them as comments here in the PR so that folks can get a feel for how it works. Thanks!

@emmanuelle
Copy link
Contributor Author

Thanks for the info and feedback @jackparmer and @jonmmease . I plan to finish working on my todo list tomorrow morning (Montréal time).

@emmanuelle emmanuelle changed the title [WIP] Ternary plot Ternary plot Jan 29, 2019
@emmanuelle
Copy link
Contributor Author

This PR is ready for review! @jonmmease

@jonmmease
Copy link
Contributor

Great, thanks @emmanuelle. I'll do a thorough review this evening (EST), and then hopefully we can get this merge in tomorrow for 3.6.0.

@jonmmease jonmmease added this to the v3.6.0 milestone Jan 29, 2019
@emmanuelle
Copy link
Contributor Author

Awesome, thanks @jonmmease !

coloring=coloring,
smoothing=smoothing)
side_trace, tick_trace = _styling_traces_ternary(x_ticks, y_ticks)
fig = go.FigureWidget(data=[contour_trace, tick_trace, side_trace],
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently all of the other figure factories return a go.Figure instance rather than a FigureWidget. For consistency this should return a Figure instance by default.

My eventual goal is for all of the figure factories to support an output_type argument that can be set to 'dict', 'Figure', or 'FigureWidget' to control how the figure is returned (See #1079). For this PR feel free to just always return a Figure, or to add the output_type argument that defaults to Figure.

@jonmmease
Copy link
Contributor

Hi @emmanuelle, I had one comment above about the return type, but otherwise your implementation looks great!

I would like to take a step back, however, and discuss this general approach a bit. Here's my summary of how this works now (please correct me if I misstate something)

  1. The input grid of 3D Barycentric (ternary) coordinates is transformed into a mesh of 2D cartesian coordinates
  2. This mesh is interpolated/upsampled into a 2d Cartesian grid.
  3. The interpolated grid points that fall outside of the ternary triangle are removed
  4. This remaining grid is used to create a standard Cartesian plotly contour trace type.
  5. The ternary axes and ticks and labels are drawn with lines and annotations.

Here are a few hangups I have with this approach:

  1. Aesthetically, it leads to this jagged fill behavior when you zoom in.
    newplot 5

  2. Plotly.js supports a nice ternary axis system that support ternary specific pan/zoom (https://plot.ly/python/ternary-plots/), so its a shame to not be able to take advantage of that.

  3. This approach won't be very composable if users want to layer scatter points or lines on top of the contours.

I'm not sure that it's practical, but I'd like to at least discuss an alternative approach. Could we do something like:

  1. (as above)
  2. (as above)
  3. (as above)
  4. Rather than construct a contour trace type, use scikit-image to find the contour lines (http://scikit-image.org/docs/dev/auto_examples/edges/plot_contours.html)
  5. Transform the contour lines back into Barycentric coordinates
  6. For each contour line, create a scatterternary trace with mode: 'lines' (https://plot.ly/python/ternary-scatter-contour/#create-ternary-contour-plot).

I think this would work fine for contours as lines, and I think we could probably even get the filled contour appearance by setting the fill to some combination of 'toself' and 'tonext'.

With this approach, it would be possible for a user to use the figure factory to build the initial contour plot, but then they could then add their own scatterternary traces on top of it.

What do you think @emmanuelle @jackparmer ?

One additional thought is the question of whether we also want this figure factory to support computing the kernel density directly from a collection of input points in Barycentric coordinates. Something analagous to the histogram2dcontour trace type (https://plot.ly/python/2d-histogram-contour/)

@emmanuelle
Copy link
Contributor Author

Great suggestion, thank you @jonmmease. I'll definitely give it a try, it will have to wait tomorrow though... If you need this figure factory for the release and you're satisfied with the API, one solution is to merge this implementation and try another implementation later on.

Regarding your other points, the input coordinates do not have to lie on a grid, they can be any set of points in barycentric coordinates (but the interpolation will be inside their convex hull only).

@jonmmease
Copy link
Contributor

Hi @emmanuelle, yeah I know this suggestion is a lot of change. Sorry I didn't process the approach enough when you first posted the WIP PR.

I'm definitely open to merging this as is and revisiting it later. If my suggested approach works out, I think we should be able to keep the same figure factory API.

@jackparmer what would you like to do? I think the near-term options are:

  1. merge as-is and release 3.6.0 in the next day or two, then choose whether or not to investigate a new approach after the release.
  2. Release 3.6.0 without this and then focus on a new approach right after the release.
  3. Hold 3.6.0 while we investigate a new approach.

@jackparmer
Copy link
Contributor

jackparmer commented Jan 30, 2019

I think that merging (and releasing 3.6.0) as-is makes sense, then pursuing the approach that you outlined that does not use the cartesian plotly.js system (only barycentric). As you pointed out, it would be awesome if we could simulate smooth contours with many toself or tonext fills.

@empet , in the meantime, would you be interested in helping to see if Jon's approach (or something similar) is viable for ternary contours (see above)?

Rather than construct a contour trace type, use scikit-image to find the contour lines (http://scikit-image.org/docs/dev/auto_examples/edges/plot_contours.html)
Transform the contour lines back into Barycentric coordinates
For each contour line, create a scatterternary trace with mode: 'lines' (https://plot.ly/python/ternary-scatter-contour/#create-ternary-contour-plot).

@nicolaskruchten
Copy link
Contributor

nicolaskruchten commented Jan 30, 2019 via email

@empet
Copy link

empet commented Jan 30, 2019

My comments on @jonmmease raised questions:

  1. if Plotly go.Contour allowed x and y as 2d arrays, with their values not necessarily ordered , then we could generate a ternary contour without jagged sides, via an ilr-transformation, typical for compositional data.

  2. To use the Plotly ternary axes provided for go.Scatterternary, we should know the cartesian coordinates of the reference triangle vertices.

  3. As long as Plotly provides contour lines in cartesian coordinates only via go.Contour, why should we call a function from scikit-image for ternary contour lines? Obviously we can use plt.contour, too

@empet
Copy link

empet commented Jan 30, 2019

@jonmmease, @jackparmer
The function skimage.measure.find_contours is of no use for ternary contour lines.
First drawback: it works only for a single level, not a list of levels http://scikit-image.org/docs/dev/api/skimage.measure.html#skimage.measure.find_contours.

Second: We have 3 meshgrids x, y, z to get the contour lines, but skimage function uses only z-values.
So it returns the coordinates of contour-lines with respect to a cartesian reference system unrelated to the system of our x, y.

In the example at the link provided by @jonmmease, http://scikit-image.org/docs/dev/auto_examples/edges/plot_contours.html
the x coordinate in meshgrid runs in the interval [-pi, pi], i.e. [-3.14, 3.14], and the contours[0] has x-coordinates in the interval [0, 9.939027340869417].

@emmanuelle
Copy link
Contributor Author

@empet we can still use the skimage function by iterating over contour values, and rescaling contour values by the grid length. I'm giving it a try at the moment, I think it's doable. More soon!

@emmanuelle
Copy link
Contributor Author

@jonmmease please tell me if I should still merge this PR, another one with the more native ternary scatter should be ready this afternoon.

@empet
Copy link

empet commented Jan 31, 2019

@emmanuelle I did it here https://plot.ly/~empet/15111, applying an isometric-log-ratio transformation to ternary points (that maps data isometrically on 2d-space), extracting contour lines via plt.contour() , that are mapped back to points on ternary contour lines. plt.contour() accepts x, y, z as arguments and a list of levels. No rescaling is needed.

@emmanuelle
Copy link
Contributor Author

@empet awesome! How can I see the code generating this plot? My problem with plt.contour was that in a notebook it generated a matplotlib figure with the contour plot.

@empet
Copy link

empet commented Jan 31, 2019

@emmanuelle I'll upload the notebook on Plotly cloud in a few minutes and post here the link. Now I'm cleaning the code.

@empet
Copy link

empet commented Jan 31, 2019

@emmanuelle This is the link to the J. notebook https://plot.ly/~empet/15113

I learned about ilr - transformation from this paper: http://ima.udg.es/~jamf/Updating%20on%20the%20KDE%20for%20Compositional%20Data_MChM_definitive.pdf
Using the method presented here I estimated the density of some ternary data https://plot.ly/~empet/15092

@empet
Copy link

empet commented Jan 31, 2019

@emmanuelle The above method to plot the contour lines works only for proportions at the moment.

@jonmmease
Copy link
Contributor

@jonmmease please tell me if I should still merge this PR, another one with the more native ternary scatter should be ready this afternoon.

@emmanuelle I was thinking of merging today, but I'm fine giving it another day or two since it sounds like you and @empet are making good progress! Why don't you open a new PR if you come up with an alternative approach, that way this one is still here as-is in case we decide to merge this now and update to a new approach later.

Thanks!

@empet
Copy link

empet commented Jan 31, 2019

@emmanuelle I updated the notebook with a short presentation of the ilr-transform method.

@emmanuelle
Copy link
Contributor Author

Thank you very much @empet !

so that we don't have to support them initially when we convert
this figure factory to represent contours using scatterternary traces.
@jonmmease
Copy link
Contributor

@emmanuelle, I wanted to make two more changes here before merging for 3.6.0

  1. I renamed create_ternarycontour as create_ternary_contour to be a bit more consistent with the other figure factories.
  2. I removed the reversescale, smoothing, and showlabels options so that we don't need to support these in Updated method for ternary contour figure factory #1418 before replacing this implementation with the scatterternary implementation.

Does this look alright to you?

@emmanuelle
Copy link
Contributor Author

@jonmmease yes it will be difficult to support showlabels, good idea to remove it. reversescale is easy to implement but I don't think it's a killer feature. OK for me!

@jonmmease
Copy link
Contributor

reversescale is easy to implement but I don't think it's a killer feature

Cool, definitely feel free to bring it back in #1418 🙂

@jonmmease jonmmease merged commit 3908788 into plotly:master Feb 1, 2019
@jonmmease jonmmease changed the title Ternary plot Ternary contour plot Feb 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants