search

umami-hep / puma

puma - Plotting UMami Api
Apache License 2.0
4 stars 27 forks source link

Fix marker size not changing when explicitly changed in __init__ of V… #243

Closed jansam123 closed 6 months ago

jansam123 commented 6 months ago

Fix marker size not changing when explicitly changed in init of VarVsVar. Added kw arguments to appropriate matplotlib plotter, also for ratio plot

Summary

During initialization of VarVsVar an explicitly passed kw argument markersize did not change the size of the markers on the plot. In the former code, there was no explicit passing of the argument to plt.scatter, plt.errorbar, and also in the legend with mpl.lines.Line2D. I added these arguments explicitly with plt.scatter(s=elem.markersize**2), plt.errorbar(ms=elem.markersize) and mpl.lines.Line2D(markersize=elem.markersize).

For some reason, in the case of plt.scatter the size of the marker scales in a different way. To make sure the sizes of markers are the same in legend and the plot, it has to be squared.

samvanstroud commented 6 months ago

Thanks for the fix @jansam123! Can you fix the failing tests?

jansam123 commented 6 months ago

Thanks for the fix @jansam123! Can you fix the failing tests?

Sure, will get to it!

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.65%. Comparing base (6b5d1b6) to head (ab81426).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #243 +/- ## ======================================= Coverage 97.65% 97.65% ======================================= Files 44 44 Lines 4569 4569 ======================================= Hits 4462 4462 Misses 107 107 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jansam123 commented 6 months ago

Thanks for the fix @jansam123! Can you fix the failing tests?

I just replaced the test image with a new one, where the markers are bigger. However, the current default value is quite big. Do you want to change it to be more similar to the one in the previous example, or keep it this way?

samvanstroud commented 6 months ago

Thanks @jansam123, the defaults should look as good as possible so sounds good to me to reduce the default a bit.

You'll also need to fix the lining and add a change log entry. Thanks!

jansam123 commented 6 months ago

I think it is ready @samvanstroud

samvanstroud commented 6 months ago

Thanks a lot @jansam123!