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 plot_doas function that only plots DOAS related information #135

Merged
merged 5 commits into from
Feb 1, 2023

Conversation

s-weigand
Copy link
Member

@s-weigand s-weigand commented Jan 29, 2023

This PR replaces the current plot_doas function which was meant as an augmented plot_overview and is broken on main with a more focused (3 plots) version that only plots DOAS-related information.
In addition, it adds a custom plot tick calculation function for values that are in the unit of Pi.

Original plots by @ism200 in the publication of the 4TT case study

image

Plots using plot_doas and the pyglotaran results

Note: The slight variations are due to a slightly different model using DOAS to fit parts of the coherent artifact for better fit results and the flipped phase for osc1 is due to using np.unwrap in pyglotaran in contrast to mod 2*Pi in the original plot
image

As for adding \overline{\nu}... as text labels to the plots this should be done via a helper function since it would need a latex environment at the user side installed, which we should not force on them to have by default.

Pseudo plot code e.g. for beta_doas
from pyglotaran_extras.plotting.plot_doas import plot_doas
from glotaran.io import load_result

result = load_result("doas_case_study/result.yaml")

fig, axes = plot_doas(
    result,
    damped_oscillation=["osc1", "osc2", "osc5"],
    time_range=(-0.8, 0.8),
    spectral=550,
    center_λ=550,
    # oscillation_type="sin"
    # normalize=False
)

Change summary

Checklist

  • ✔️ Passing the tests (mandatory for all PR's)
  • 🧪 Adds new tests for the feature (mandatory for ✨ feature and 🩹 bug fix PR's)

@s-weigand s-weigand requested a review from a team as a code owner January 29, 2023 23:50
@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Jan 29, 2023

Sourcery Code Quality Report

❌  Merging this PR will decrease code quality in the affected files by 1.87%.

Quality metrics Before After Change
Complexity 3.31 ⭐ 3.13 ⭐ -0.18 👍
Method Length 96.33 🙂 96.30 🙂 -0.03 👍
Working memory 7.81 🙂 10.08 😞 2.27 👎
Quality 69.34% 🙂 67.47% 🙂 -1.87% 👎
Other metrics Before After Change
Lines 547 672 125
Changed files Quality Before Quality After Quality Change
pyglotaran_extras/__init__.py 84.86% ⭐ 83.57% ⭐ -1.29% 👎
pyglotaran_extras/plotting/plot_doas.py 45.89% 😞 33.35% 😞 -12.54% 👎
pyglotaran_extras/plotting/utils.py 75.07% ⭐ 75.31% ⭐ 0.24% 👍
tests/plotting/test_utils.py 82.08% ⭐ 80.22% ⭐ -1.86% 👎

Here are some functions in these files that still need a tune-up:

File Function Complexity Length Working Memory Quality Recommendation
pyglotaran_extras/plotting/plot_doas.py plot_doas 6 ⭐ 380 ⛔ 22 ⛔ 32.05% 😞 Try splitting into smaller methods. Extract out complex expressions
pyglotaran_extras/plotting/utils.py select_plot_wavelengths 3 ⭐ 136 😞 11 😞 59.58% 🙂 Try splitting into smaller methods. Extract out complex expressions
tests/plotting/test_utils.py test_abs_max 0 ⭐ 136 😞 5 ⭐ 76.11% ⭐ Try splitting into smaller methods

Legend and Explanation

The emojis denote the absolute quality of the code:

  • ⭐ excellent
  • 🙂 good
  • 😞 poor
  • ⛔ very poor

The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request.


Please see our documentation here for details on how these metrics are calculated.

We are actively working on this report - lots more documentation and extra metrics to come!

Help us improve this quality report!

@github-actions
Copy link
Contributor

Binder 👈 Launch a binder notebook on branch s-weigand/pyglotaran-extras/plot-doas

@codecov
Copy link

codecov bot commented Jan 29, 2023

Codecov Report

Base: 43.25% // Head: 44.97% // Increases project coverage by +1.72% 🎉

Coverage data is based on head (3e96084) compared to base (c1385c9).
Patch coverage: 30.43% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #135      +/-   ##
==========================================
+ Coverage   43.25%   44.97%   +1.72%     
==========================================
  Files          29       29              
  Lines         904      916      +12     
  Branches      133      133              
==========================================
+ Hits          391      412      +21     
+ Misses        506      497       -9     
  Partials        7        7              
Impacted Files Coverage Δ
pyglotaran_extras/plotting/plot_doas.py 31.81% <18.91%> (+31.81%) ⬆️
pyglotaran_extras/plotting/utils.py 29.35% <75.00%> (+3.61%) ⬆️
pyglotaran_extras/__init__.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@s-weigand
Copy link
Member Author

@ism200 @jsnel I'm not 100% sure if we want to keep the slitting between spectral (wavelength we select the oscillations data from) and center_λ (wavelength we select the IRF to shift the oscillations).
Maybe using a default of None for center_λ and replacing it with the value of spectral in that case would be the best of two worlds, but in that case it would kinda hide the dispersion from the user if they were only looking at the DOAS with our plot functions.

Copy link
Member

@jsnel jsnel left a comment

Choose a reason for hiding this comment

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

Much improved!

@jsnel jsnel requested a review from ism200 January 31, 2023 22:30
@jsnel
Copy link
Member

jsnel commented Jan 31, 2023

For posterity I record that @ism200 reviewed these changes and acknowledges the (subtle) difference between the two plots that are shown, as captured by this comment:

Note: The slight variations are due to a slightly different model using DOAS to fit parts of the coherent artifact for better fit results and the flipped phase for osc1 is due to using np.unwrap in pyglotaran in contrast to mod 2*Pi in the original plot

He commented that the unwrap method is in fact better, so from my perspective this is good to be merged in.

If you wish to acknowledge this @ism200, please press the Files changed button on top and review as 'ok'.

@s-weigand
Copy link
Member Author

New plot functionality

Pseudo plot code e.g. for ex_doas_beta
from pyglotaran_extras.plotting.plot_doas import plot_doas
from glotaran.io import load_result

result = load_result("doas_case_study/result.yaml")

fig, axes = plot_doas(
    result,
    damped_oscillation=["osc1", "osc2", "osc5"],
    time_range=(-0.8, 0.8),
    spectral=550,
    # oscillation_type="sin"
    # normalize=True
)

for vline_pos in [412]:
    axes[1].axvline(vline_pos, color="k", linewidth=1)
    axes[2].axvline(vline_pos, color="k", linewidth=1)

image

@ism200
Copy link
Contributor

ism200 commented Feb 1, 2023

this looks all very nice now. thanks a lot!

Copy link
Contributor

@ism200 ism200 left a comment

Choose a reason for hiding this comment

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

thanks a lot !

@s-weigand s-weigand merged commit 137ad2a into glotaran:main Feb 1, 2023
@s-weigand s-weigand deleted the plot-doas branch February 1, 2023 18:55
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