-
Notifications
You must be signed in to change notification settings - Fork 239
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
Adding 2d histogram function #5249
Conversation
You're the lucky winner @jbloom22
|
hail/python/hail/plot/plots.py
Outdated
@@ -126,6 +127,147 @@ def cumulative_histogram(data, range=None, bins=50, legend=None, title=None, nor | |||
return p | |||
|
|||
|
|||
def set_font_size(p, font_size: str = "12pt"): | |||
"""Set as many font sizes as possible in a bokeh figure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Set all possible font sizes in a bokeh figure."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I'm not 100% sure that these are all of the possible ones. Just the ones I've encountered so far. I'll make this a sub-function of the histogram2d one for now
hail/python/hail/plot/plots.py
Outdated
Parameters | ||
---------- | ||
p : :class:`bokeh.plotting.figure.Figure` | ||
Input Figure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lower-case figure
hail/python/hail/plot/plots.py
Outdated
p : :class:`bokeh.plotting.figure.Figure` | ||
Input Figure. | ||
font_size : str | ||
String of font size in points (e.g. "12pt"). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
single-quote more pythonic?
plot_title='2-D histogram', plot_width=600, plot_height=600, font_size='7pt', | ||
colors=bokeh.palettes.all_palettes['Blues'][7][::-1]): | ||
"""Plot a 2-D histogram of x vs y, which are NumericExpressions from a Table. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add Examples section illustrating how the parameters are used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually give examples in a section that starts with:
Examples
---------
e.g.
https://hail.is/docs/0.2/methods/impex.html#hail.methods.import_bgen
hail/python/hail/plot/plots.py
Outdated
check_row_indexed('histogram_2d', x) | ||
check_row_indexed('histogram_2d', y) | ||
if source != y_source: | ||
raise ValueError(f"histogram_2d expects two expressions of 'Table', found {source} and {y_source}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean, on the same table?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this check up a couple lines
|
||
grouped_ht = source.group_by( | ||
x=hail.str(x_levels.find(lambda w: x >= w)), y=hail.str(y_levels.find(lambda w: y >= w)) | ||
).aggregate(c=hail.agg.count()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the right indentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My PyCharm claims it is but happy to change if there's another preferred one
hail/python/hail/plot/plots.py
Outdated
font_size : str | ||
String of font size in points (default "7pt"). | ||
colors : List[str] | ||
List of hex colors from low to high. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you more explanation (or a link) regarding how this is used
I think if there are elements outside the range you should warn |
hail/python/hail/plot/plots.py
Outdated
y_levels = hail.literal(list(frange(y_range[0], y_range[1], y_spacing))[::-1]) | ||
|
||
grouped_ht = source.group_by( | ||
x=hail.str(x_levels.find(lambda w: x >= w)), y=hail.str(y_levels.find(lambda w: y >= w)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why strings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little funky. Bokeh needs them to be strings in order to correctly plot them as discrete values rather than a continuous scale.
Also willing to discuss a default color palette - anyone want to have a principled discussion about some nice defaults? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few small tweaks, otherwise looks great!
hail/python/hail/plot/plots.py
Outdated
bins=int, x_bins=nullable(int), y_bins=nullable(int), | ||
plot_title=nullable(str), plot_width=int, plot_height=int, | ||
font_size=str, colors=sequenceof(str)) | ||
def histogram_2d(x, y, x_range=None, y_range=None, bins=40, x_bins=None, y_bins=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
numpy and matplotlib don't have the _
. I'd remove it for consistency
https://docs.scipy.org/doc/numpy/reference/generated/numpy.histogram2d.html
https://matplotlib.org/api/_as_gen/matplotlib.pyplot.hist2d.html
hail/python/hail/plot/plots.py
Outdated
def histogram_2d(x, y, x_range=None, y_range=None, bins=40, x_bins=None, y_bins=None, | ||
plot_title='2-D histogram', plot_width=600, plot_height=600, font_size='7pt', | ||
colors=bokeh.palettes.all_palettes['Blues'][7][::-1]): | ||
"""Plot a 2-D histogram. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plot a two-dimensional histogram.
plot_title='2-D histogram', plot_width=600, plot_height=600, font_size='7pt', | ||
colors=bokeh.palettes.all_palettes['Blues'][7][::-1]): | ||
"""Plot a 2-D histogram of x vs y, which are NumericExpressions from a Table. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually give examples in a section that starts with:
Examples
---------
e.g.
https://hail.is/docs/0.2/methods/impex.html#hail.methods.import_bgen
hail/python/hail/plot/plots.py
Outdated
Number of bins on x-axis, will override ``bins`` if provided. | ||
y_bins : int | ||
Number of bins on y-axis, will override ``bins`` if provided. | ||
plot_width : int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why prefix these three with plot
?
hail/python/hail/plot/plots.py
Outdated
String of font size in points (default '7pt'). | ||
colors : List[str] | ||
List of colors (hex codes, or strings as described | ||
`here <https://bokeh.pydata.org/en/latest/docs/reference/colors.html>`__). Effective with one of the many |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Compatible" rather than "Effective"?
No description provided.