Skip to content

Add initial set of metrics#19

Merged
smoia merged 21 commits intophysiopy:masterfrom
tsalo:add-metrics
Feb 25, 2021
Merged

Add initial set of metrics#19
smoia merged 21 commits intophysiopy:masterfrom
tsalo:add-metrics

Conversation

@tsalo
Copy link
Member

@tsalo tsalo commented Nov 13, 2020 •
edited by smoia
Loading

Closes #2 and closes #3 and closes #7.

Proposed Changes

  • Add duecredit and a references file to collect duecredit citations.
  • Add/draft functions for the cardiac (CRF) and respiratory (RRF) response functions.
  • Add RPV, ENV, and RV metrics.
  • Add RETROICOR.

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!

@tsalo tsalo marked this pull request as draft November 13, 2020 14:43
tsalo
tsalo commented Dec 30, 2020
View reviewed changes
phys2denoise/metrics/chest_belt.py
Comment on lines +110 to +112
out_samplerate : :obj:`float`
Sampling rate for the output time series in seconds.
Corresponds to TR in fMRI data.
Copy link
Member Author

@tsalo tsalo Dec 30, 2020

Choose a reason for hiding this comment

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

Do we want to include downsampling in the individual metric functions, or have it in a separate step?

Copy link
Member Author

@tsalo tsalo Jan 8, 2021

Choose a reason for hiding this comment

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

I'm thinking that downsampling should be done somewhere else TBH.

@tsalo tsalo changed the title Add initial set of metrics Add RV, RPV, ENV, RRF, and CRF Jan 1, 2021
@tsalo tsalo mentioned this pull request Jan 4, 2021
Merged
11 tasks
@tsalo tsalo added the Minormod This PR generally closes an "Enhancement" issue. It increments the minor version (0.+1.0) label Jan 8, 2021
@tsalo tsalo added the Enhancement New feature or request label Feb 11, 2021
@tsalo tsalo added the Skip release This PR preserves the current version when merged, and doesn't appear in the changelog label Feb 25, 2021
@tsalo tsalo marked this pull request as ready for review February 25, 2021 17:12
@tsalo tsalo changed the title Add RV, RPV, ENV, RRF, and CRF Add initial set of metrics Feb 25, 2021
@smoia smoia added Minormod-breaking For development only, this PR increments the minor version (0.+1.0) but breaks compatibility and removed Minormod This PR generally closes an "Enhancement" issue. It increments the minor version (0.+1.0) Skip release This PR preserves the current version when merged, and doesn't appear in the changelog labels Feb 25, 2021
@smoia smoia assigned smoia and tsalo Feb 25, 2021
@smoia smoia merged commit fcab9f3 into physiopy:master Feb 25, 2021
@tsalo tsalo deleted the add-metrics branch February 25, 2021 18:13
@smoia
Copy link
Member

smoia commented Feb 25, 2021

🚀 PR was released in 0.2.0 🚀

@smoia smoia added the released This issue/pull request has been released label Feb 25, 2021
rzlim08
rzlim08 reviewed Feb 25, 2021
View reviewed changes
phys2denoise/metrics/chest_belt.py
----------
.. [1] C. Chang & G. H. Glover, "Relationship between respiration,
end-tidal CO2, and BOLD signals in resting-state fMRI," Neuroimage,
issue 4, vol. 47, pp. 1381-1393, 2009.
Copy link
Contributor

@rzlim08 rzlim08 Feb 25, 2021

Choose a reason for hiding this comment

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

A bit of a nit-pick, but I think RV was described in "Influence of heart rate on the BOLD signal: the cardiac response function" also by Chang and Glover in 2009. It may also be in this paper, but I didn't see it from a skim.

Copy link
Member Author

@tsalo tsalo Feb 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think @smoia noticed similar issues with my citations (#31). We can address in a new PR, or you can fold it into #21?

Copy link
Contributor

@rzlim08 rzlim08 Feb 25, 2021

Choose a reason for hiding this comment

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

I think we should handle it in a new PR

@tsalo tsalo mentioned this pull request Feb 26, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@rzlim08 rzlim08 rzlim08 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@tsalo tsalo

@smoia smoia

Labels

Enhancement New feature or request Minormod-breaking For development only, this PR increments the minor version (0.+1.0) but breaks compatibility released This issue/pull request has been released

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Requested metric: respiratory pattern variability Requested metric: respiratory variance

4 participants

@tsalo @smoia @rzlim08 @CesarCaballeroGaudes