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 coherent artifact plot functionality #123

Merged
merged 5 commits into from
Jan 1, 2023

Conversation

s-weigand
Copy link
Member

@s-weigand s-weigand commented Dec 29, 2022

This change adds a new plot function plot_coherent_artifact which implements #33.

Here are some example plots for the DOAS example (currently our only example that uses the coherent artifact):
image
image

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)

Closes issues

Not sure for which other plots #33 applies, so we might want to factor out the scaling and normalization into another helper function thus I'm not marking it as closed.

@s-weigand s-weigand requested review from jsnel and ism200 December 29, 2022 20:09
@s-weigand s-weigand requested a review from a team as a code owner December 29, 2022 20:09
@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Dec 29, 2022

Sourcery Code Quality Report

✅  Merging this PR will increase code quality in the affected files by 0.56%.

Quality metrics Before After Change
Complexity 2.66 ⭐ 2.56 ⭐ -0.10 👍
Method Length 70.00 🙂 75.88 🙂 5.88 👎
Working memory 7.44 🙂 7.21 🙂 -0.23 👍
Quality 75.69% 76.25% 0.56% 👍
Other metrics Before After Change
Lines 394 454 60
Changed files Quality Before Quality After Quality Change
pyglotaran_extras/__init__.py 86.21% ⭐ 84.86% ⭐ -1.35% 👎
pyglotaran_extras/plotting/utils.py 74.88% 🙂 75.07% ⭐ 0.19% 👍
tests/plotting/test_utils.py 85.39% ⭐ 82.08% ⭐ -3.31% 👎

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

File Function Complexity Length Working Memory Quality Recommendation
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-irfas

@codecov
Copy link

codecov bot commented Dec 29, 2022

Codecov Report

Base: 40.61% // Head: 40.20% // Decreases project coverage by -0.40% ⚠️

Coverage data is based on head (b808f0c) compared to base (6894d63).
Patch coverage: 37.03% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #123      +/-   ##
==========================================
- Coverage   40.61%   40.20%   -0.41%     
==========================================
  Files          24       25       +1     
  Lines         719      771      +52     
  Branches       98      108      +10     
==========================================
+ Hits          292      310      +18     
- Misses        427      460      +33     
- Partials        0        1       +1     
Impacted Files Coverage Δ
...glotaran_extras/plotting/plot_coherent_artifact.py 23.80% <23.80%> (ø)
pyglotaran_extras/plotting/utils.py 25.74% <81.81%> (+5.09%) ⬆️
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.

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.

Looks good, corrected some small imperfections in doc strings.

pyglotaran_extras/plotting/plot_coherent_artifact.py Outdated Show resolved Hide resolved
pyglotaran_extras/plotting/plot_coherent_artifact.py Outdated Show resolved Hide resolved
pyglotaran_extras/plotting/plot_coherent_artifact.py Outdated Show resolved Hide resolved
pyglotaran_extras/plotting/plot_coherent_artifact.py Outdated Show resolved Hide resolved
pyglotaran_extras/plotting/utils.py Outdated Show resolved Hide resolved
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 sebastian. this function is key to the Lego-paper. indeed (the concept of) this function should also work with different input, e.g. concentrations and SA(D)S, or damped oscillations and DOAS. let us give that a try when we zoom again. if indeed we can generalize, then the name can become more generic. we will of course then need arguments to indicate what we want to plot. i foresee now three types of input: (1) IRF derivatives and IRFAS, (2) damped oscillations and DOAS, (3) concentrations and SA(D)S. Since these are all fixed pairs, one named arguments suffices.

@s-weigand
Copy link
Member Author

Since factoring out the scaling would be just an internal change and affect the user facing API this could be done in a l8er PR.

@jsnel jsnel merged commit bdb5a48 into glotaran:main Jan 1, 2023
@s-weigand s-weigand deleted the plot-irfas branch January 2, 2023 21:16
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