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

Fix 1474 - add fields to result object - correct calculation of number of clps. #1484

Merged
merged 4 commits into from
Jul 13, 2024

Conversation

joernweissenborn
Copy link
Member

@joernweissenborn joernweissenborn commented Jul 6, 2024

Fixes #1474

  • This adds additional_penalty to the result object and renames nr_clp back to number_of_clps.
  • Correct degrees_of_freedom calculation.

@joernweissenborn joernweissenborn requested review from jsnel, a team and s-weigand as code owners July 6, 2024 15:14
Copy link
Contributor

github-actions bot commented Jul 6, 2024

Binder 👈 Launch a binder notebook on branch joernweissenborn/pyglotaran/staging_1474

@joernweissenborn joernweissenborn changed the base branch from main to staging July 6, 2024 15:14
Copy link
Contributor

sourcery-ai bot commented Jul 6, 2024

🧙 Sourcery has finished reviewing your pull request!


Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

We've reviewed this pull request using the Sourcery rules engine. If you would also like our AI-powered code review then let us know.

Which is more consistent with 0.7 API
Copy link

codecov bot commented Jul 6, 2024

Codecov Report

Attention: Patch coverage is 87.50000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 85.2%. Comparing base (2d0fc21) to head (0884198).
Report is 14 commits behind head on staging.

Files with missing lines Patch % Lines
glotaran/optimization/optimization.py 50.0% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           staging   #1484   +/-   ##
=======================================
  Coverage     85.2%   85.2%           
=======================================
  Files           91      91           
  Lines         3735    3756   +21     
  Branches       726     734    +8     
=======================================
+ Hits          3184    3203   +19     
- Misses         438     440    +2     
  Partials       113     113           

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

@jsnel
Copy link
Member

jsnel commented Jul 13, 2024

Evaluated PR running study_fluorescence\fluorescence_global_and_target_analysis.ipynb.

On main:
image
Before this PR:
image
AFter this PR:
image

Conclusion: this does indeed resolve issue 1474.

@jsnel jsnel changed the title Fix 1474 Fix 1474 - add fields to result object - correct calculation of number of clps. Jul 13, 2024
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.

Reviewed and tested ok.

Closes #1474

@jsnel jsnel merged commit c176c78 into glotaran:staging Jul 13, 2024
34 of 35 checks passed
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.

[staging] 🐛 Add number_of_clps to result and correct degrees_of_freedom calculation
2 participants