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

refactor: factor get_plottables logic out of histplot #534

Merged
merged 1 commit into from
Oct 31, 2024

Conversation

0ctagon
Copy link
Member

@0ctagon 0ctagon commented Oct 31, 2024

Idea is to have a get_plottables() to use internally in the future plothist function. Most of the things that created the Plottables in histplot have been moved in get_plottables() or to their own little function that get_plottables() is calling.
Then, we can create and manage Plottable objects, so we can easily remove boost-histogram dependency.
The returned Plottable objects have a new argument _has_variances to store the info if they had variances before the error is automatically calculated when creating the Plottable object (before it was only calculated using .to_errorbar just before plotting).

This line:
self.variances = self.values if not self._has_variances else self.variances
in Plottable.errors() is necessary as we query the variances a lot in plothist to compute uncertainties. I have concerns that this line might create problems (e.g. re-weighted histograms) but all the tests are passing for now. We might see differences when using it with plothist examples? maybe self.variances = self.values if not self.variances else self.variances is safer?

I'm also not sure about this change in hist_object_handler():

    elif isinstance(hist, PlottableHistogram):
        # Before >> msg = "Cannot give bins with existing histogram"
        # Before >> raise TypeError(msg)
        # Now    >> hist_obj = hist

I didn't see a reason for this error.

@andrzejnovak @jonas-eschle @cyrraz

@andrzejnovak
Copy link
Member

Thanks @0ctagon! We previously wanted disallow passing bins as an arg, when passing hist object to avoid conflicting information.

The refactor makes sense to me and separates the custom plottable making logic outside of the histplot function which is good.

We'll see if this causes any issues down the road, but if the tests pass I think we're good.

@andrzejnovak andrzejnovak changed the title feat: refactor histplot + new get_plottables function refactor: factor get_plottables logic out of histplot Oct 31, 2024
@andrzejnovak andrzejnovak merged commit 3077cde into scikit-hep:main Oct 31, 2024
12 checks 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
Development

Successfully merging this pull request may close these issues.

2 participants