✨ Add support for pfid megacomplex #1510
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
jsnel
force-pushed
the
pfid_megacomplex
branch
from
fbf1aa2 to
cacafec
Compare
jsnel
force-pushed
the
pfid_megacomplex
branch
from
19cc2ed to
c555dab
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1510 +/- ##
=======================================
+ Coverage 88.6% 88.8% +0.1%
=======================================
Files 107 109 +2
Lines 5128 5245 +117
Branches 962 982 +20
=======================================
+ Hits 4544 4658 +114
Misses 468 468
- Partials 116 119 +3 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
|
🧙 Sourcery has finished reviewing your pull request! Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
s-weigand
force-pushed
the
pfid_megacomplex
branch
from
91e3db6 to
2e6d3f8
Compare
1 task
jsnel
force-pushed
the
pfid_megacomplex
branch
from
2e6d3f8 to
e19c83b
Compare
|
Removed the bugfix from this PR and into its own PR: #1512 |
jsnel
force-pushed
the
pfid_megacomplex
branch
from
e19c83b to
cfbd353
Compare
ism200
reviewed
Aug 21, 2024
s-weigand
requested changes
Aug 23, 2024
2 tasks
This commit adds the `PFIDMegacomplex` class along with its dependencies and related classes. The `PFIDMegacomplex` is a megacomplex used for fitting perturbed free induction decay as described in Hamm 1995: https://doi.org/10.1016/0301-0104(95)00262-6 Co-authored-by: Sebastian Weigand <s.weigand.phy@gmail.com> Co-authored-by: Jörn Weißenborn <Joern.weissenborn@gmail.com> Co-authored-by: Ivo van Stokkum <ism200@users.noreply.github.com> ✨ Refactor return argument calculate_pfid_matrix_gaussian_irf Keep the (online) linters happy ✨ Refactor frequency conversion in PFIDMegacomplex Postpone the frequency conversion ✨ Refactor: Update PFID megacomplex matrix initialization The code changes in this commit refactor the initialization of the `matrix` variable in the `PFIDMegacomplex` class. Instead of setting all elements to 1, it is now initialized with all elements set to 0. Additionally, an exception is raised if the `irf` parameter is None, indicating that an IRF (Impulse Response Function) is required for PFID megacomplex calculations. Note: The removed function `calculate_pfid_matrix_no_irf()` was not used and has been commented out. Guard against zero standard errors for fixed parameters alpha fix for alpha alpha moved to freq diff introduced 2nd oscillation shifted by alpha[0] pfid starting from Hamm (1995) but with excited state decaying with kd small correction kd=15 fixed 1 bug call to calculate_decay_matrix_gaussian_irf_on_index and added spectral_axis_inverted option still problem with large times with left_shifted_axis thus avoiding overflows, and extending the right time range kappa introduced extra decay rate cleaning after trying several things, alpha is now unused cleaning comments removing the alpha related comments new b computation using left_shifted_axis_indices intermediate stage 1st attempt to unify pfid with doas after reintroducing the phase with pfid
Ran pre-commit Commented out some unused code
In the current version of the case studies the kappa and gamma parameters is unused, this for simplicity we have removed it from the initial (simple) pfid implementation. Non-relevant comments have been removed.
Working unit tests: - OneOscillationWithIrf - OneOscillationWithSequentialModel
By switching m1 and m2 we reliably introduce the ordering bug, which is then fixed by the fix for matrix provider.
It doesn't belong in virtual environment but should be coming from global.
jsnel
force-pushed
the
pfid_megacomplex
branch
from
b680554 to
dd657e8
Compare
|
s-weigand
approved these changes
Aug 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add support for the modelling of perturbed free induction decay (PFID) as described in the 1995 paper by P. Hamm.
Change summary
The feature will be documented as part of an upcoming publication.
Checklist