scikit-hep / mplhep

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

fix: flow bins xticklabel correction #502

Closed atownse2 closed 3 weeks ago

atownse2 commented 1 month ago

The issues that this PR fixes are described in #501. This includes:

Here are some plots I can make with this PR: 5cfac6c0-34c9-41c2-b7b9-05105b80e6d1

9b02ff13-326c-4a9c-bf1d-975d92816e04

atownse2 commented 1 month ago

Also, to get the dashed lines and diamonds to draw correctly I needed to draw them on both the top and bottom of each axis. The diamond on the top of the top axis got in the way of labels so I removed it but I left the dashed line because I liked it. I'm not sure if this is the best choice in general.

andrzejnovak commented 4 weeks ago

Hey, thanks for the PR and sorry for the delay in reviewing this. This looks mostly good to me, but I am not sure I understand why the dashed line needs to appear on top of the plot as well.

atownse2 commented 4 weeks ago

Hi, yes the dashed lines don't need to appear on the top. I thought that it made the overflow bin more clear. What I will do is only include the fixes in this PR and then I will make another PR with some drawing changes that I like.

andrzejnovak commented 4 weeks ago

Thanks, so this now needs to update the reference images. Check https://github.com/scikit-hep/mplhep/blob/master/CONTRIBUTING.md and let me know if you have a problem getting the pytest generation running. N.b. the images generated by pytest might register as "new" in git even if they didn't change, please don't add those to the commit to limit the repo size.

andrzejnovak commented 3 weeks ago

Thanks a lot @atownse2! I'll mint a new release with the fix shortly.