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: top right corner position, inside the frame, with loc=5 parameter #423

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

asantra
Copy link

@asantra asantra commented Apr 20, 2023

Addition of loc parameter value 5.
If loc=5, the experiment label and lumi text are plotted on the top right-hand side, inside the plot frame.

@andrzejnovak andrzejnovak changed the title top right corner position, inside the frame, with loc=5 parameter feat: top right corner position, inside the frame, with loc=5 parameter Apr 21, 2023
@andrzejnovak
Copy link
Member

andrzejnovak commented Apr 21, 2023

Welcome @asantra. Can you add it to the test https://github.com/scikit-hep/mplhep/blob/master/tests/test_styles.py#L216 ?
Maybe reshape the grid as 2x3 instead of 1x5.

I am a bit curious how the alignment will look like.

@asantra
Copy link
Author

asantra commented Apr 22, 2023

Hi @andrzejnovak
I have added the new location in the tests.
Please have a look and let me know if anything needs to be changed.

Best,
Arka

@andrzejnovak
Copy link
Member

Please add the updated test image as well CONTRIBUTING.md (don't add the unchanged images, git likes to flag them as different)

@asantra
Copy link
Author

asantra commented Apr 22, 2023

Hi @andrzejnovak ,
Here is the updated test image:
https://github.com/asantra/mplhep/blob/arka-dev/tests/baseline/test_label_loc.png

The new style is shown in the sixth frame (bottom right).

Best,
Arka

@andrzejnovak
Copy link
Member

Sorry about the broken GHA reporting, a fix should already be on the way pytest-dev/pytest-github-actions-annotate-failures#70. Checking the repo out locally it seems there are font differences. If you run pytest locally on master, do you get passing tests?

@asantra
Copy link
Author

asantra commented Apr 28, 2023

Hi @andrzejnovak ,
I tested the tests/test_styles.py on the master branch and this is the result:

pytest tests/test_styles.py

======================================================================test session starts =======================================================================
platform darwin -- Python 3.9.7, pytest-6.2.4, py-1.10.0, pluggy-0.13.1
Matplotlib: 3.4.3
Freetype: 2.10.4
rootdir: /Users/arkasantra/HepProjects/myEdits/mplhep
plugins: anyio-2.2.0, mock-3.10.0, mpl-0.16.1, check-2.1.4
collected 36 items

tests/test_styles.py sssss..sssssssssssssssssssssssssssss [100%]

================================================================= 2 passed, 34 skipped in 1.61s ==================================================================

I think that most of the tests are skipped because I am not using a linux system.
How should I test only the relevant part (i.e. test_label_loc() function in test_styles.py)?

Best,
Arka

@andrzejnovak
Copy link
Member

What about the test tests in test_basic.py? We currently strip most text for the comparisons, but some of them have inherent bin labels such as https://github.com/scikit-hep/mplhep/blob/master/tests/baseline/test_hist2dplot.png.

Note if you ran with the generate option, you might already have new "reference" files produced with the local font, so the tests won't fail locally.

You can run a specific function in pytest with pytest tests/test_styles.py::test_label_loc()

@asantra
Copy link
Author

asantra commented Apr 29, 2023

Hi @andrzejnovak ,
The test_basic.py fails on my system.
Here is the short summary:

==================================================================== short test summary info =====================================================================
FAILED tests/test_basic.py::test_histplot_flow - AttributeError: 'StepPatch' object has no property 'flow'
FAILED tests/test_basic.py::test_histplot_hist_flow - AttributeError: 'StepPatch' object has no property 'flow'
FAILED tests/test_basic.py::test_histplot_uproot_flow - AttributeError: 'StepPatch' object has no property 'flow'
FAILED tests/test_basic.py::test_histplot_type_flow - AttributeError: 'StepPatch' object has no property 'flow'
FAILED tests/test_basic.py::test_hist2dplot_flow - AttributeError: 'QuadMesh' object has no property 'flow'
================================================================== 5 failed, 46 passed in 8.37s ==================================================================

Here is the full output:
https://docs.google.com/document/d/1wXfWkzklqpR1g-5-pm2aje-fpW67xDdmhV0fjyiWY5k/edit?usp=sharing

It seems like some software mismatch in my system.

Best,
Arka

@andrzejnovak
Copy link
Member

The errors you're getting now are about a recently merged PR, can you rebase on master and try again?

@asantra
Copy link
Author

asantra commented May 12, 2023

Hi @andrzejnovak ,
I am sorry; I did not understand your instruction.
I cloned the master branch from
scikit-hep/mplhep
in a separate area.
I put my edited files in that new area.
Then I did the pytest:
pytest tests/test_basic.py

. Still, I am getting the same set of errors are before:
https://docs.google.com/document/d/1wXfWkzklqpR1g-5-pm2aje-fpW67xDdmhV0fjyiWY5k/edit?usp=sharing

I will appreciate if you help me out here.
May I ask for the exact git commands that I need to follow?

Best,
Arka

@jonas-eschle
Copy link
Contributor

Hi @asantra , apologies for the delay: can you locally rebase on master (or merge?). Do you need any help with that?

If you've done that, can you push, so that we see all the changes in the PR

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.

3 participants