search

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

feat: Adding publication/supplemental info to label #466

Closed yimuchen closed 4 months ago

yimuchen commented 4 months ago

test_loc2_dataFalse test_loc2_dataTrue

andrzejnovak commented 4 months ago

Thanks for the quick turn-around. Could you run this with this test https://github.com/scikit-hep/mplhep/blob/048b2b43f93d2d2415ae7f795d4354015caae906/tests/test_styles.py#L216 so we can check all locs at once? https://github.com/scikit-hep/mplhep/blob/master/tests/baseline/test_label_loc.png

I'd say for locs 1-3 the optimal location is obvious, less so for 0,4

yimuchen commented 4 months ago

Right, still a bunch of work needs to be done for many configurations... Will need quite a bit of tweaking.

test_pub_loc

yimuchen commented 4 months ago

Updated for decent 1,2,3 settings, Not entirely sure what to do for 0 and 4 for the very extreme cases. test_pub_loc

andrzejnovak commented 4 months ago

Thanks! I think for loc=4 the SuppText should go on a new line like in loc=2. For loc=0 I am not sure. Perhaps in-frame or the right edge where the z-axis/cbar label would be.

yimuchen commented 4 months ago

Updated for loc=0 and 4. Made sure that changes to 4 did not change the result without the arXiv ID.

test_label_loc test_pub_loc

yimuchen commented 4 months ago

I just checked with a friend in ALTAS, I don't think they have a prefered location for arXiv labels (and if they do, I guess they can contribute to change this). So let me know if I should do any additional adjustments to these current defaults.

test_pub_loc test_label_loc

yimuchen commented 4 months ago

@andrzejnovak What do you think about this new tweaked result? test_pub_loc

andrzejnovak commented 4 months ago

Thanks a lot! I have a last comment, for loc=0 the orientation of the supptext should be flipped (such that if a person turns their head left, they can read both the supptext and y-axis label. Otherwise LGTM

yimuchen commented 4 months ago

Done! test_pub_loc

andrzejnovak commented 4 months ago

@yimuchen it looks like the reference image is missing for the test

yimuchen commented 4 months ago

Should I generate it from my side? Or should we have some CI process to generate these images to ensure consistency?

andrzejnovak commented 4 months ago

The file should be generated locally and included in the tests/baseline. The CI will then check against it

https://github.com/scikit-hep/mplhep/blob/master/CONTRIBUTING.md#generating-reference-visuals

andrzejnovak commented 4 months ago

Thanks a lot @yimuchen. Let me mint a release.