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

Plot utils #5251

Merged
merged 4 commits into from Feb 7, 2019
Merged

Plot utils #5251

merged 4 commits into from Feb 7, 2019

Conversation

@cseed
Copy link
Collaborator

@cseed cseed commented Feb 5, 2019

I'm not sure if this is worth it, but gives the plotting library a slightly more self-contained feel.

Copy link
Collaborator

@tpoterba tpoterba left a comment

I'm fine with this and do agree it's a QoL improvement, but want to raise the point that learning Hail's plotting library is not sufficient for someone's visualization needs - a user needs to learn bokeh (or something else), or will feel extremely constrained by the narrow set of things Hail can do at the moment. I do worry that hiding bokeh in this way will preempt that learning process.

I think we should merge this, but want to hear your thoughts first.

@cseed
Copy link
Collaborator Author

@cseed cseed commented Feb 7, 2019

I agree completely. I certainly don't think we can hide or replace Bokeh (I hope the explicit emphasis on Bokeh in the documentation makes this clear), but I think we should continue to add common-case utilities to hl.plot life easier for users (and ourselves).

@cseed cseed dismissed tpoterba’s stale review Feb 7, 2019

addressed comment

@danking danking merged commit ff97acf into hail-is:master Feb 7, 2019
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants