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

feat: compatibility with mplhep #177

Open
wants to merge 21 commits into
base: mpldev
Choose a base branch
from

Conversation

andrzejnovak
Copy link

This is still a draft, but I am opening it now to get CI running...

@0ctagon 0ctagon self-requested a review October 7, 2024 03:10
@0ctagon
Copy link
Collaborator

0ctagon commented Oct 8, 2024

In the end I think we want merge this into mpldev branch and not main.

@0ctagon
Copy link
Collaborator

0ctagon commented Oct 11, 2024

After reviewing the new svgs without modifying mplhep (fa01534), I see 3 main differences, and I fixed 2 of them in the last commit (c0a124b) by modifying a bit mplhep

  1. When stacked histograms are plotted, ax.legend() is not putting them in the right order in the legend (top to bottom). This was easily fixed in feat: correct ax.legend() order for stack hists scikit-hep/mplhep#527

  2. By default, step style will display variances of the Plotttable if they exist. As discussed here, this behavior is good for the user. For the documentation, I think we want to use simple without errors histograms, to not overload the plots.

2.5. A side effect of the step style is the representation in the legend, which is a line in mplhep but a rectangle in matplotlib. This was fixed in scikit-hep/mplhep#529

  1. The plots where the histograms are next to each others is not as straight forward, but it can be added with a custom style keyword like step or fill. This was also implemented in feat: added bar type scikit-hep/mplhep#529

PS1: I changed the _kwargs = soft_update_kwargs(_kwargs, {"linewidth": 1.}) (from 1.2) in mplhep step style to match the same style as plothist.

PS2: In plot_two_hist_comparison, the # xlim = (h1.axes[0].edges[0], h1.axes[0].edges[-1]) being commented is a bit problematic, as the top and bottom plot doesn't have the same x_range.

PS3: Pytest not agreeing is because it is making the plots with the released version of matplotlib, not the one I modified locally. We could put those modifications on a mpldev branch in mplhep and install this branch in the gha.

PS4: It's really nice that it agrees quite well with little modifications 😃

@0ctagon
Copy link
Collaborator

0ctagon commented Oct 16, 2024

Replacing plot_hist_uncertainties was straight forward, I only needed to modify in mplhep:

        err_defaults = {
            "linestyle": "none",
            "marker": ".",
            "markersize": 6.0, <------
            "elinewidth": 1.5, <------
        }

and no plot changed.

For plot_error_hist, I kept the asymmetric uncertainty calculation method, it was also quite straight forward with this style change in mplhep:

        band_defaults = {
            "edgecolor": "dimgrey", <----
            "hatch": "////", <----
            "fill": False, <----
            "lw": 0, <----
        }

I think more work is needed to see how our yerr calculation and the w2method overlap.

@0ctagon
Copy link
Collaborator

0ctagon commented Oct 17, 2024

2a45b94 implemented histtype=side in mplhep. By default it is filled, but it also supports step and step with yerr representation.

@0ctagon
Copy link
Collaborator

0ctagon commented Dec 18, 2024

I updated the figures to correspond to the output of the current mplhep/plothist code. I think some default parameters could be changed, like the dot size or the grayness of the hashing. I would also change our default histograms with something without errors by default, so it doesn't overcrowd the plots. But for now every examples are working well with only using mplhep as a backend for plotting. I think we should merge this before opening another PR to focus on removing boost-histogram dependency (which will be easier to review with this PR merged already).

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