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

* Added interactive scatter and joint plots in experimental #5601

Merged
merged 13 commits into from Mar 21, 2019

Conversation

lfrancioli
Copy link
Contributor

Both of these support a number of features not available in hail.plot:

  • interactive legend (click to hide/show elements)
  • labelling with continuous expressions
  • labelling using multiple expressions (will display a dropdown selection widget)
  • specifying color schemes
  • hovering on points displays their coordinates, labels and additional source fields
  • legend is also displayed outside of the plotting space to be unobtrusive

Both of these support a number of features not available in hail.plot:
- interactive legend (click to hide/show elements)
- labelling with continuous expressions
- labelling using multiple expressions (will display a dropdown selection widget)
- specifying color schemes
- hovering on points displays their coordinates, labels and additional source fields
- legend is also displayed outside of the plotting space to be unobtrusive
@tpoterba tpoterba self-assigned this Mar 14, 2019
@tpoterba
Copy link
Contributor

the whole plot module is experimental, so those can just go there (they're certainly documented just fine too!)

can we just replace the default scatter with this one?

@lfrancioli
Copy link
Contributor Author

Sure, the interface is slightly different though to accommodate for some of the new features. It also doesn't support passing python lists instead of hail expr -- could be trivially added if useful though.

@tpoterba
Copy link
Contributor

I think that's fine!

@lfrancioli
Copy link
Contributor Author

OK, I've moved it and made the interface as close as I could to the previous scatter. One thing is the default value for n_divisions. It was 500 before, now I've set it to None (i.e. no downsampling). I'm fine either way, but it seems somewhat more intuitive to me for the default to be no downsampling.

List of x-values to be plotted.
y : :class:`.NumericExpression`
List of y-values to be plotted.
label : Dict[str, :class:`.Expression`]
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be hard to extend this to support the original interface of label=mt.population?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that was trivial actually! I've pushed a revised version that supports this. I did not add support for List[label] though (this was used when providing python lists rather than hail exp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, in the same vein, let me return a Figure in case no label or a single label is passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! So this is now mostly back compatible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could re-add the legend option to if desired

Copy link
Contributor

Choose a reason for hiding this comment

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

nah that's OK

* Renamed fields to be as close as possible to previous scatter plot names
* Added marker size option

Removed unused import +  space from experimental/plots
* Small join_plot change: assign gray color to missing factors in provided color palette rather than throwing an error
- `source_fields` -> `hover_fields` (clearer, same as `manhattan`)
- removed `collect_all`, use `n_divisions is None` instead
- Set `n_divisions` to previous default value of 500 everywhere
* small fixes, cleaning ups
* Fixed an issue with non-numeric, non-string types for hover fields
* Also corrected indentation of scatter doc
@tpoterba
Copy link
Contributor

I'm not sure you signed up for this haha

@danking danking merged commit fd6ade6 into hail-is:master Mar 21, 2019
@lfrancioli
Copy link
Contributor Author

Hahaha, I really did not! Had a bunch of things that were working that I wanted to add to experimental, and then someone suggested I should just replace plot.scatter instead 😛

@tpoterba
Copy link
Contributor

😉

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

3 participants