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

Add default rcParams #386

Merged
merged 3 commits into from
Feb 5, 2024
Merged

Add default rcParams #386

merged 3 commits into from
Feb 5, 2024

Conversation

iaraota
Copy link
Collaborator

@iaraota iaraota commented Nov 15, 2023

This pull request introduces default rcParams, ensuring that plots have the desired appearance without the necessity of activating the ligo-summary-3.10 Conda environment.

These parameters were obtained by comparing the matplotlib.rcParams differences between igwn and ligo-summary-3.10 Conda environments.

@iaraota iaraota requested a review from eagoetz November 15, 2023 17:57
@iaraota iaraota self-assigned this Nov 15, 2023
@iaraota
Copy link
Collaborator Author

iaraota commented Jan 29, 2024

@eagoetz I tested the following:

import matplotlib.pyplot as plt
import numpy as np
import matplotlib as mpl

plt.rcParams["figure.figsize"] = (5,10)
data = np.random.randn(50)
plt.plot(data)
plt.savefig('before_gwsumm.png')
import gwsumm
plt.close('all')
plt.plot(data)
plt.savefig('before_after.png')

The saved figures remain unchanged, that is, the "figure.figsize" stays at (5,10) rather than the GWSumm default of (12, 6).

@eagoetz
Copy link
Collaborator

eagoetz commented Jan 29, 2024

@iaraota Thanks for trying this out! Have you tried from gwsumm import plot? I want to confirm what that does with this PR. Thanks!

Also, I'm not sure what's going on with the CI failures, but it is likely related to this change.

Copy link

codecov bot commented Feb 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a50504f) 50.17% compared to head (09601f5) 49.96%.
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #386      +/-   ##
==========================================
- Coverage   50.17%   49.96%   -0.21%     
==========================================
  Files          60       60              
  Lines        8685     8729      +44     
==========================================
+ Hits         4357     4361       +4     
- Misses       4328     4368      +40     
Flag Coverage Δ
Linux 49.96% <100.00%> (-0.21%) ⬇️
python3.10 49.96% <100.00%> (-0.21%) ⬇️
python3.11 49.96% <100.00%> (-0.21%) ⬇️
python3.9 49.96% <100.00%> (-0.21%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@iaraota
Copy link
Collaborator Author

iaraota commented Feb 1, 2024

@eagoetz thank you for your suggestion. I tested the following, and the plots remain the same:

import matplotlib.pyplot as plt
import numpy as np
import matplotlib as mpl

plt.rcParams["figure.figsize"] = (5,10)
data = np.random.randn(50)
plt.plot(data)
plt.savefig('before_gwsumm.png')
from gwsumm import plot
plot.DataPlot(['L1:DMT-SNSL_EFFECTIVE_RANGE_MPC'], 1388102418, 1388188818)
plt.close('all')
plt.plot(data)
plt.savefig('before_after.png')

I fixed the test. It was checking whether cp.set('plot', 'figure.figsize', '2, 1') works. However, since rcParams used to be empty, is was asserting the rcParams as a dictionary. Now I am testing only the change in figure.figsize. By the way, this test also shows that rcParams is changeable, as you were concerned it could not be.

Copy link
Collaborator

@eagoetz eagoetz left a comment

Choose a reason for hiding this comment

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

LGTM

@eagoetz eagoetz added this to the 2.2.1 milestone Feb 2, 2024
@eagoetz eagoetz merged commit 3d59f66 into gwpy:master Feb 5, 2024
10 checks passed
@iaraota iaraota deleted the default_rcParams branch April 23, 2024 20:38
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