-
Notifications
You must be signed in to change notification settings - Fork 25
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
feat: plot1d for hist(cat,reg) #174
Conversation
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.
Mostly looks good!
src/hist/basehist.py
Outdated
if self.ndim == 1: | ||
len_cat = np.sum([ax.traits.discrete for ax in self.axes]) | ||
if self.ndim == 1 or (self.ndim == 2 and len_cat == 1): |
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.
I'm not sure I like this. It's also valid to make a 2D plot where one axis is categorical (or two, for that matter). I think it would be more predictable if .plot()
on a 2D histogram always made a 2D plot. But I could be convinced. What do other people think? @nsmith-, @matthewfeickert, perhaps?
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.
What @nsmith- 's coffea API does is have overlay keyword to control this for non-discrete axes, but if think the number of 2d histograms of type (sample, quantity) is gonna be higher than (quantity, quantity) at least if one of the axes is StrCat (or just Cat)
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.
I suppose in coffea its either plot1d
or plot2d
. But indeed if overlay: Optional[str]
is a parameter, one can take that to decide if a 2D hist is a 2D plot or a set of 1D plots with the overlay axis as specified.
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.
Another option might be to follow pandas style, where the hist would have to be turned into some kind of HistGroupBy
object before plotting, e.g. h.groupby("region").plot()
.
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.
tbh since hist
does plot1d()
and plot2d()
as well and plot()
is just a two char shortcut, this is a trifle.
@nsmith- can you think of any other use for some kind of groupby fcn than this? I don't think it's worth fiddling with just for this
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.
Not yet, probably you are right to leave it as-is for now.
for more information, see https://pre-commit.ci
@henryiii should I try to make sourcery happy? otherwise this is ready for review I think |
Only if it wants a sensible change (not always). If it's just the "guard" style if, I mildly like that, so if you are neutral, I'd go with that. |
Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>
@all-contributors please add @andrzejnovak for code |
I've put up a pull request to add @andrzejnovak! 🎉 |
for more information, see https://pre-commit.ci
Sourcery Code Quality Report✅ Merging this PR will increase code quality in the affected files by 0.12%.
Here are some functions in these files that still need a tune-up:
Legend and ExplanationThe emojis denote the absolute quality of the code:
The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request. Please see our documentation here for details on how these metrics are calculated. We are actively working on this report - lots more documentation and extra metrics to come! Let us know what you think of it by mentioning @sourcery-ai in a comment. |
@henryiii looks done then, let me know if you'd like me to squash the commits |
Nah, that's fine, I usually squash and merge unless there's a need to leave more than one comment (like a moved and changed file, or a set of sequential steps). |
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.
Looks good to me!
I'll fill in the changelog later, to keep the PRs from colliding. |
Thanks!!! |
Fixes #173.
I am kind of expecting to get shouted at for UHI abuse one way or another, but that's what I could piece from the docs on first run.