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

fix: flow bins xticklabel correction #502

Merged
merged 7 commits into from
Jun 10, 2024
Merged

fix: flow bins xticklabel correction #502

merged 7 commits into from
Jun 10, 2024

Conversation

atownse2
Copy link

@atownse2 atownse2 commented Jun 3, 2024

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

  • Use the correct flow bin index when drawing overflow xticklabels
  • Draw flow xticklabel on overflow bin center instead of using default xticks
  • Add support for shared axes: Draw overflow bin marker and dashed line on all shared axes, excluding the marker on top of top axis.

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

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

@atownse2
Copy link
Author

atownse2 commented Jun 3, 2024

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.

for h in [0, 1]:
_ax.set_xticks(xticks)
_ax.set_xticklabels(xticklabels)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you move the skip condition up here, wouldn't that prevent drawing the dashed line on top?

@andrzejnovak
Copy link
Member

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
Copy link
Author

atownse2 commented Jun 7, 2024

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
Copy link
Member

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 andrzejnovak changed the title Flow bins xticklabel fix fix: flow bins xticklabel correction Jun 10, 2024
@andrzejnovak
Copy link
Member

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

@andrzejnovak andrzejnovak merged commit 11d70df into scikit-hep:master Jun 10, 2024
11 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