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: Adding publication/supplemental info to label #466

Merged
merged 11 commits into from
Feb 19, 2024

Conversation

yimuchen
Copy link
Contributor

@yimuchen yimuchen commented Feb 8, 2024

  • First running version of adding publication information to the plot labels. (Works decently well for loc=2)
  • Additional adjustments will need to done for all possible loc/data combinations

test_loc2_dataFalse
test_loc2_dataTrue

@andrzejnovak
Copy link
Member

Thanks for the quick turn-around. Could you run this with this test

def test_label_loc():
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
Copy link
Contributor Author

yimuchen commented Feb 8, 2024

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

test_pub_loc

@yimuchen
Copy link
Contributor Author

yimuchen commented Feb 8, 2024

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

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

yimuchen commented Feb 9, 2024

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

yimuchen commented Feb 9, 2024

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 yimuchen changed the title draft: Adding publication information Adding publication information Feb 9, 2024
Copy link
Member

@andrzejnovak andrzejnovak left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Implementation-wise looks good to me. For loc=0 I was rather thinking this could live along the y-axis, outside of the figure like this:
image

I'd also say the spacing in loc 1,2,3 is a bit tight (in 2,3 this can be seen in the test with some symbols overlaping).

For loc my preference would probably be for the SuppText to be below the xsec information.

What do you think?

@yimuchen
Copy link
Contributor Author

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

@andrzejnovak
Copy link
Member

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

Done!
test_pub_loc

@andrzejnovak
Copy link
Member

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

@yimuchen
Copy link
Contributor Author

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

@andrzejnovak
Copy link
Member

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 andrzejnovak changed the title Adding publication information feat: Adding publication/supplemental info to label Feb 19, 2024
@andrzejnovak andrzejnovak merged commit 9ea493f into scikit-hep:master Feb 19, 2024
9 checks passed
@andrzejnovak
Copy link
Member

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

jonas-eschle pushed a commit that referenced this pull request Apr 22, 2024
* First running version (additional tweaking for text offset is required)

* Added test case for publication information

* Changing canvas size

* Fixed styling for loc 1,3

* Updating styling for loc=0 and 4

* Tighting the distance for loc=1

* Removed unneeded variable

* Tweaking positions

* Changed direction

* Adding reference image

---------

Co-authored-by: Andrzej Novak <[email protected]>
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