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

Introduce Hail version of ggplot #11247

Merged
merged 91 commits into from
Jan 25, 2022
Merged

Conversation

johnc1231
Copy link
Contributor

No description provided.

johnc1231 and others added 30 commits December 6, 2021 15:16
added bar graphs and histograms
…rate lines, and geom_point geom_line and geom_text all use same color handling code
hail/python/hail/gg/geoms.py Outdated Show resolved Hide resolved
self.breaks = breaks
self.labels = labels

def update_axis(self, fig):
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems like its not meant to be used only by subclasses, right? Let's prefix it with an underscore to indicate that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's meant to be used by GGPlot in order to update the axes of a plot with things like ticks, labels, etc.

@typecheck_method(contig=str)
def _contig_global_position(self, contig):
@property
def _global_positions_dict(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Any reason to not make this part of the public API (make it not underscore)? If we're referencing it in the plotting functionality, then it must be generally useful, yeah?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot this was in there, modified quickly as part of a "genomic" scale experiment. Will clean up and make it not _.

Copy link
Contributor

@danking danking left a comment

Choose a reason for hiding this comment

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

a few drive-by thoughts. It'd be great to add type annotations as well! I find they're useful when exploring a new library, but I realize they're pretty rare in Hail Query code.

@johnc1231
Copy link
Contributor Author

Addressed @danking's comments, though I'm not going to attempt to add type information in this PR, given intended timeline for release and other commitments.

@johnc1231 johnc1231 mentioned this pull request Jan 24, 2022
Copy link
Collaborator

@patrick-schultz patrick-schultz left a comment

Choose a reason for hiding this comment

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

This is really great! I know we want to release this today, so just a few small things.

hail/python/hail/docs/ggplot/index.rst Outdated Show resolved Hide resolved
hail/python/hail/ggplot/ggplot.py Outdated Show resolved Hide resolved
hail/python/hail/ggplot/ggplot.py Show resolved Hide resolved
hail/python/hail/ggplot/geoms.py Outdated Show resolved Hide resolved
hail/python/hail/ggplot/geoms.py Outdated Show resolved Hide resolved

if scale.is_discrete() and not is_discrete_type(dtype):
raise ValueError(f"Aesthetic {aes_key} has continuous dtype but non continuous scale")
if not scale.is_discrete() and is_discrete_type(dtype):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this check not is_continuous_type(dtype) instead? More generally, is_discrete_type and is_continuous_type are not complements, and it's not clear to me if that is intentional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is true. Just went and added a is_continuous method on scale to address this. I think they were briefly complementary though that was an oversimplification

@johnc1231
Copy link
Contributor Author

@patrick-schultz Addressed your comments, and made the docs a little better.

Copy link
Collaborator

@patrick-schultz patrick-schultz left a comment

Choose a reason for hiding this comment

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

🎉

@johnc1231 johnc1231 merged commit 4be7f44 into hail-is:main Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants