scikit-hep / mplhep

Extended histogram plotting on top of matplotlib and HEP collaboration compatible styling
https://mplhep.readthedocs.io
MIT License
189 stars 66 forks source link

refactor: factor get_plottables logic out of histplot #534

Closed 0ctagon closed 2 weeks ago

0ctagon commented 2 weeks ago

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 commented 2 weeks ago

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.