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

[FEATURE] HistStack #169

Open
henryiii opened this issue Mar 24, 2021 · 7 comments
Open

[FEATURE] HistStack #169

henryiii opened this issue Mar 24, 2021 · 7 comments
Assignees
Labels
enhancement New feature or request

Comments

@henryiii
Copy link
Member

henryiii commented Mar 24, 2021

A possible design for stacked histograms is the following:

A HistStack holds multiple histograms; axes are required to match. Calling .plot(...) forwards to mplhep, just like Hist.plot, but with the stack of histograms as a list in the first argument.

To make a stack, you could use |, so that:

(h1 | h2 | h3).plot()

Would make a stack of histograms and plot it.

This also might tie into #165, possibly .interp on a stack would interpolate between histograms? Just a thought.

EDIT: should have added to #34 instead...

@henryiii henryiii added the enhancement New feature or request label Mar 24, 2021
@henryiii henryiii changed the title [FEATURE] HistogramStack [FEATURE] HistStack Mar 24, 2021
@eduardo-rodrigues
Copy link
Member

@henryiii, just thinking out loud, why a "|" rather than a "&"?

BTW, did you look at some of the functionality that was put into histbook in the past, before it got archived? Stacked histograms were also covered there and one may "steal" ideas from it.

@henryiii
Copy link
Member Author

Because it's used in a lot of other places in Python now to mean union, such as sets, dicts (3.9+), and types (3.10+). I believe a "Book" in HistBook was a different thing, a set of different histograms that could be filled together, while this is a set of matching histograms that must have been filled differently. There was a "stack" method, but I think it was just a display style for a single histogram. Good idea to look at HistBook for (other, too) ideas!

@eduardo-rodrigues
Copy link
Member

I see, fair enough for the "|", then.

@henryiii
Copy link
Member Author

FYI: In Python 3.10, int | str is the same as from typing import Union; Union[int, str], and it will work inside isinstance, so you can write isinstance(x, int | str) instead of isinstance(x, (int, str)).

@henryiii
Copy link
Member Author

henryiii commented Jun 25, 2021

hist.stacks.Stack(h1, h2) would be the explicit constructor. There also should be some way to convert a category axis to a stack. h.stack("axis1") -> stack. The current plot shortcut could be written h.stack("cat1").plot().

This was referenced Jun 28, 2021
@henryiii henryiii mentioned this issue Jul 7, 2021
4 tasks
henryiii added a commit that referenced this issue Jul 7, 2021
* feat: begin histstack

* Update .pre-commit-config.yaml

* feat: make HistStack work

* Modify checks for categorical axis

* Add check for matching axes types

* feat & test: add a judgement for Hist.plot and tests for stacks

* fix: skip Stack tests for Python 3.6

* fix: make axes match for Stack

* feat: change Stack plotting implementation

* test: remove tests for stack with different axes

* feat: check mplhep dependency and allow Stack(unnamed_hist, named_hist)

* feat: allow stack(ax1, ax2) but not allow plot, and without tests

* fix: change the axes check back

* feat: h.stack(name/idx)

* test: add test for Stack reprs

* test: add some tests for h.stack and Stack(axes)

* fix: solve the circular import problem

* refactor: working on Stack

Co-authored-by: Aman Goel <aman.goel185@gmail.com>
Co-authored-by: Henry Schreiner <henryschreineriii@gmail.com>
@LovelyBuggies
Copy link
Collaborator

For implicit constructor, aka s = h1 | h2 | h3, we probably need to overload __or__ for both Hist and Stack. Hence, IMO, s.push(h4) could be helpful (and s.pop() correspondingly).

@henryiii
Copy link
Member Author

henryiii commented Aug 9, 2021

I'm thinking 95% of users will likely do h.stack(ax) and not Stack(hist1, hist2, ...), so I'm not that worried about adding a shortcut for it now. If we do, yes, but let's not add it unless we find a strong need for it. Things like #274 are more pressing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants