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

Updated Legends to appear on ax instead of fig #1272

Merged
merged 22 commits into from
Oct 1, 2024

Conversation

missing-user
Copy link
Contributor

@missing-user missing-user commented Sep 26, 2024

Resolves #1271 "Legends should appear on the ax object when passed to plotting functions"
Resolves #1280 "Plotting Unit Test Regression"

  • Changed default legend positioning from fig to ax, to better support using DESC functions in subplots
  • Passing an array of phi values to plot_boundaries plots cross-sections for each phi value, instead of skipping the last one.
  • Reduced tolerances for plotting unit tests
  • Updated plotting baseline

@missing-user missing-user changed the title Fix legends appearing on fig instead of ax #1271 Fix legends appearing on fig instead of ax Sep 26, 2024
Copy link
Collaborator

@YigitElma YigitElma left a comment

Choose a reason for hiding this comment

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

Thanks you for your help @missing-user !

tests/test_plotting.py Show resolved Hide resolved
tests/baseline/test_plot_boundaries.png Outdated Show resolved Hide resolved
Philipp Jurasic added 4 commits September 30, 2024 13:48
- Capture the regression in test_plot_boundaries behavior from 7b6b5a3
- Update baseline for newly failing plotting tests
Copy link

codecov bot commented Sep 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.45%. Comparing base (056fcc4) to head (3e1bd18).
Report is 23 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1272      +/-   ##
==========================================
+ Coverage   95.43%   95.45%   +0.01%     
==========================================
  Files          96       96              
  Lines       23791    23791              
==========================================
+ Hits        22706    22709       +3     
+ Misses       1085     1082       -3     
Files with missing lines Coverage Δ
desc/plotting.py 96.18% <100.00%> (+0.07%) ⬆️

... and 1 file with indirect coverage changes

Copy link
Member

@f0uriest f0uriest left a comment

Choose a reason for hiding this comment

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

Tests pass locally for me with reduced tolerances. Would be good if a few other people can check as well, we've had issues in the past where if the tolerances are too tight we get a lot of false positives just due to hardware differences.

@YigitElma
Copy link
Collaborator

Tests also pass for me but I think I have the same hardware as @f0uriest . Can @dpanici or @kianorr tests this on Mac?

@YigitElma
Copy link
Collaborator

And please don't mind the benchmark action, our current CI doesn't allow PRs from forks to add automatic comments (which benchmark does at the end)

@YigitElma
Copy link
Collaborator

Resolve issue #1271 "Legends should appear on the ax object when passed to plotting functions"

Can you change the description to include "Resolves #1280" ?

@missing-user missing-user changed the title Fix legends appearing on fig instead of ax Resolves #1280 Sep 30, 2024
@missing-user missing-user changed the title Resolves #1280 Updated Legends to appear on ax instead of fig, Resolves https://github.com/PlasmaControl/DESC/issues/1271, Resolves https://github.com/PlasmaControl/DESC/issues/1280 Sep 30, 2024
@missing-user missing-user changed the title Updated Legends to appear on ax instead of fig, Resolves https://github.com/PlasmaControl/DESC/issues/1271, Resolves https://github.com/PlasmaControl/DESC/issues/1280 Updated Legends to appear on ax instead of fig, Resolves https://github.com/PlasmaControl/DESC/issues/1271 , Resolves https://github.com/PlasmaControl/DESC/issues/1280 Sep 30, 2024
@missing-user missing-user changed the title Updated Legends to appear on ax instead of fig, Resolves https://github.com/PlasmaControl/DESC/issues/1271 , Resolves https://github.com/PlasmaControl/DESC/issues/1280 Updated Legends to appear on ax instead of fig Sep 30, 2024
@missing-user
Copy link
Contributor Author

Sorry, thought you meant the title for a second... I think everything is ready to merge now, thanks for the help!

@dpanici
Copy link
Collaborator

dpanici commented Oct 1, 2024

works fine for me as well

@YigitElma YigitElma merged commit 9096947 into PlasmaControl:master Oct 1, 2024
22 of 24 checks passed
@YigitElma
Copy link
Collaborator

@missing-user Thanks for the contribution!

@missing-user missing-user deleted the fix-plotting-subplots branch October 1, 2024 17:26
@missing-user missing-user restored the fix-plotting-subplots branch October 1, 2024 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants