Skip to content

Conversation

smoia
Copy link
Member

@smoia smoia commented Jul 28, 2025

Closes #

Turns out (or so I think) we were computing RETROICOR in a horrible (if not wrong) way due to a few oversights from reviewers (me included) and code authors.
In fixing the issues, I decided to just rewrite everything to improve computation and allow for interactive effects to be computed.

Proposed Changes

Phases:

  • phases computation functions do not return physio objects nor accept them as as inputs to simplify interaction with main retroicor script.
  • phases computation functions are now in a submodule of its own.
  • phases are computed at the sampling rate of the physiological data, not of the neuroimaging data.
  • phases computation is now vectorialised to make it more efficient. This changes it a little from the original formulae but it should be equivalent.
    Consequently, RETROICOR:
  • retroicor computation function still accepts physio objects, both from physutils and from peakdet, or numpy arrays, and still returns accordingly, but it does not use the usual decorator for that (instead it handles it internally).
  • retroicor computation function now accept both cardiac and respiratory data at the same time, thus allowing for interaction regressors to be computed as well.
  • if at least one physutils.Physio object was given as input, the function returns the physutils.Physio object that were provided with the computed metics in the metrics metadata. If the other entry was an array, it will still return a new object based on that array.
  • otherwise, is a peakdet.Physio or an array-like data was given as input, the function returns the metric itself.
  • the metric is always a dictionary with two dictionaries inside, one for retroicor, one for the phases; these are two dictionaries themselves, with one entry per slice time.

Tagging @CesarCaballeroGaudes @afni-dglen @mrikasper because of your expertise with RETROICOR and its implementation in code. Tagging @m-miedema and @me-pic for your expertise on phys2denoise.

Change Type

  • bugfix (+0.0.1)
  • minor (+0.1.0)
  • major (+1.0.0)
  • refactoring (no version update)
  • test (no version update)
  • infrastructure (no version update)
  • documentation (no version update)
  • other

Checklist before review

  • I added everything I wanted to add to this PR.
  • [Code or tests only] I wrote/updated the necessary docstrings.
  • [Code or tests only] I ran and passed tests locally.
  • [Documentation only] I built the docs locally.
  • My contribution is harmonious with the rest of the code: I'm not introducing repetitions.
  • My code respects the adopted style, especially linting conventions.
  • The title of this PR is explanatory on its own, enough to be understood as part of a changelog.
  • I added or indicated the right labels.
  • I added information regarding the timeline of completion for this PR.
  • Please, comment on my PR while it's a draft and give me feedback on the development!

@smoia smoia self-assigned this Jul 28, 2025
@smoia smoia added the Minormod-breaking For development only, this PR increments the minor version (0.+1.0) but breaks compatibility label Jul 28, 2025
@smoia
Copy link
Member Author

smoia commented Jul 28, 2025

Also @alaurasparsi this is of interest for you!

@smoia smoia changed the title Complete overwrite of RETROICOR computation Complete overwrite of RETROICOR computation to allow multiple physiological modalities and their interactions Jul 29, 2025
@smoia smoia moved this to PR needs review in Physiopy Jul 31, 2025
@smoia smoia added this to Physiopy Jul 31, 2025
smoia and others added 2 commits August 1, 2025 16:18
Co-authored-by: Stefano Moia <[email protected]>
Co-authored-by: alaurasparsi <[email protected]>
Co-authored-by: Stefano Moia <[email protected]>
Co-authored-by: alaurasparsi <[email protected]>
@github-actions github-actions bot added the Testing This is for testing features, writing tests or producing testing code. label Aug 1, 2025
Copy link

codecov bot commented Aug 1, 2025

Codecov Report

❌ Patch coverage is 8.08081% with 91 lines in your changes missing coverage. Please review.
✅ Project coverage is 44.55%. Comparing base (34be772) to head (4fad72f).
⚠️ Report is 17 commits behind head on master.

Files with missing lines Patch % Lines
phys2denoise/metrics/multimodal.py 4.47% 64 Missing ⚠️
phys2denoise/metrics/phase.py 15.62% 27 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
- Coverage   53.85%   44.55%   -9.31%     
==========================================
  Files           8        9       +1     
  Lines         596      606      +10     
==========================================
- Hits          321      270      -51     
- Misses        275      336      +61     
Files with missing lines Coverage Δ
phys2denoise/metrics/cardiac.py 35.00% <ø> (-19.35%) ⬇️
phys2denoise/metrics/chest_belt.py 92.59% <ø> (+0.21%) ⬆️
phys2denoise/metrics/phase.py 15.62% <15.62%> (ø)
phys2denoise/metrics/multimodal.py 12.00% <4.47%> (-14.83%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Retroicor regressors are organised in a dictionary of two dictionaries, one for
retroicor regressors and one for phases. These dictionaries have one entry per
slice.
All entries are lists, always organised: cardiac regressors first, then
Copy link
Member Author

Choose a reason for hiding this comment

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

Makes more sense to invert order to follow input order

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Minormod-breaking For development only, this PR increments the minor version (0.+1.0) but breaks compatibility Testing This is for testing features, writing tests or producing testing code.
Projects
Status: PR needs review
Development

Successfully merging this pull request may close these issues.

1 participant