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/hexbin mapbox #2559

Merged
merged 14 commits into from
Jul 14, 2020
Merged

Conversation

RenaudLN
Copy link

Feature: hexbin_mapbox

Creates an hexagonal binning of lat/lon points and aggregates for a metric.

Documentation PR

  • I've seen the doc/README.md file
  • This change runs in the current version of Plotly on PyPI and targets the doc-prod branch OR it targets the master branch
  • If this PR modifies the first example in a page or adds a new one, it is a px example if at all possible
  • Every new/modified example has a descriptive title and motivating sentence or paragraph
  • Every new/modified example is independently runnable
  • Every new/modified example is optimized for short line count and focuses on the Plotly/visualization-related aspects of the example rather than the computation required to produce the data being visualized
  • Meaningful/relatable datasets are used for all new examples instead of randomly-generated data where possible
  • The random seed is set if using randomly-generated data in new/modified examples
  • New/modified remote datasets are loaded from https://plotly.github.io/datasets and added to https://github.com/plotly/datasets
  • Large computations are avoided in the new/modified examples in favour of loading remote datasets that represent the output of such computations
  • Imports are plotly.graph_objects as go / plotly.express as px / plotly.io as pio
  • Data frames are always called df
  • fig = <something> call is high up in each new/modified example (either px.<something> or make_subplots or go.Figure)
  • Liberal use is made of fig.add_* and fig.update_* rather than go.Figure(data=..., layout=...) in every new/modified example
  • Specific adders and updaters like fig.add_shape and fig.update_xaxes are used instead of big fig.update_layout calls in every new/modified example
  • fig.show() is at the end of each new/modified example
  • plotly.plot() and plotly.iplot() are not used in any new/modified example
  • Hex codes for colors are not used in any new/modified example in favour of these nice ones

Code PR

  • I have read through the contributing notes and understand the structure of the package. In particular, if my PR modifies code of plotly.graph_objects, my modifications concern the codegen files and not generated files.
  • I have added tests (if submitting a new feature or correcting a bug) or
    modified existing tests.
  • For a new feature, I have added documentation examples in an existing or
    new tutorial notebook (please see the doc checklist as well).
  • I have added a CHANGELOG entry if fixing/changing/adding anything substantial.

@nicolaskruchten
Copy link
Contributor

Thank you very much for this pull request, it looks awesome! I should have some feedback for you in a couple of days.

@nicolaskruchten nicolaskruchten added this to the 4.9.0 milestone Jun 22, 2020
@nicolaskruchten
Copy link
Contributor

OK, so I think we'd like to merge this in time for the 4.9 release in a few weeks, but one change I would propose would be to include this under plotly.figure_factories rather than plotly.express. In general, ff is where we put code that does a lot of Python-level computation, and doesn't map directly onto an underlying trace type. In this case, there's a fair bit of math happening in Python and we're using the choroplethmapbox trace type, so ff feels like a better fit than px.

What do you think?

PS: overall this is an excellent piece of work, and I can imagine reusing some of these utility functions elsewhere in the codebase, especially for automatic zoomlevel calculations etc, so "bravo"!

@RenaudLN
Copy link
Author

At first I hesitated between plotly express and figure factory but it felt to me like figure factory was for more complex plots with multiple elements. Also I made this PR so that the call to hexbin_mapbox respects the plotly.express api, you can even use the animation frames part to see the evolution of the binning. It also reuses the express docstring maker and phrasing.

Now I understand that there is not a traditional mapping between plotly.express and plotly.graph_objs with this one so it'll be your call where you want to put it 😄

And glad that some of the util functions can be reused!

@nicolaskruchten
Copy link
Contributor

OK, after some thought, I think I would like this to be in the figure_factory module, yes. We're also going to want to have some tests that lock down the behaviour of this function, similar to the ones that test px. Basically you can create a fig with some sample data and make some assertions about the resulting structure.

I'm a little worried about the use of the private build_dataframe function here, as this one saw a lot of churn recently. Hopefully it's under control now but you never know. So I'd definitely like some tests around the hexbin function that would break if I break something inside build_dataframe later :)

Finally, we'll also need a new documentation page with some examples of how to use it. This can just be a new Markdown file in /doc/python or a notebook if you're not into using Jupytext: I'll convert it for you. Once that's done I can add some links to that page from various other doc pages to drive some traffic to it :)

@RenaudLN
Copy link
Author

RenaudLN commented Jul 9, 2020

OK, in that case, is it possible to call functions from express in figure_factory or these two should be completely independent and I should only call functions from the lower-level graph_objects?
More specifically, I am currently using choropleth_mapbox as well as the build_dataframe and make_docstring from express and I'm wondering if they can stay if I move this to figure_factory.

@nicolaskruchten
Copy link
Contributor

I think it's totally fine for a figure factory to call public functions from PX like px.choropleth_mapbox. I'm a little less comfortable with calling functions like make_docstring and build_dataframe but I think it's OK in this case, as the alternative would be much worse :)

@RenaudLN
Copy link
Author

I added some tests. For now, I put them in tests/test_optional/test_figure_factory/test_figure_factory.py, not sure if that's the right place for them to be run automatically.
Also, some orca dendrogram tests fail though I have not touched this part of the code.

I'll get on to the doc page soon.

@nicolaskruchten
Copy link
Contributor

Thanks! The dendrogram error will be fixed soon: #2618

@RenaudLN
Copy link
Author

RenaudLN commented Jul 11, 2020

Documentation page added!
Just one question regarding the thumbnail in the meta, I'm not sure where to put it, is it in this repository somewhere?
I created one in case it needs to be uploaded by the Plotly team.
hexbin_mapbox

@nicolaskruchten
Copy link
Contributor

This looks great, thank you! I'll do a couple of fixups in an additional commit and this will be released with 4.9 this week.

@nicolaskruchten
Copy link
Contributor

Finishing up in #2638 if you want to keep track :)

@nicolaskruchten
Copy link
Contributor

The thumbnail needs to go up in our S3 bucket, and I've just uploaded it there, FYI. https://images.plot.ly/plotly-documentation/thumbnail/hexbin_mapbox.jpg

@nicolaskruchten nicolaskruchten merged commit b79333a into plotly:master Jul 14, 2020
@RenaudLN
Copy link
Author

Thanks for keeping me updated and for the final tweaks 👍

@nicolaskruchten
Copy link
Contributor

The thanks go to you, for one of the most complete and obviously-awesome external pull requests I've ever seen :)

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.

2 participants