Add RVT algorithm (Birn et al. 2006)

[rzlim08]

Draft algorithm of RVT.

Proposed Changes

Closes #1

• Add draft of RVT
• Add corresponding tests

Change Type

• minor (+0.1.0)

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]

Thank you for opening this! I have a couple of minor notes, but I will hopefully be able to review the code in detail in the next week or so.

This PR targets #1. Would you mind linking to it with a keyword in your summary comment?

Additionally, I believe that our current plan is to have modules based on the modality, rather than a given metric (although I haven't been on the last couple of dev calls, and @smoia may need to correct me). Assuming that's still the case, it would be awesome if you could place the rvt function in metrics/chest_belt.py instead of metrics.rvt.py.

[rzlim08]

Great, this should be done. I figured it should be in there, but wasn't sure we weren't setting ourselves up for a bit of a hairy merge in the future.

Let me know if you have any comments.

[tsalo]

It looks like the linter is catching some issues with the docstring. Would you mind addressing those? I'll paste them here for easy reference:

phys2denoise/metrics/chest_belt.py:1:1: D100 Missing docstring in public module
phys2denoise/metrics/chest_belt.py:6:1: D202 No blank lines allowed after function docstring
phys2denoise/metrics/chest_belt.py:6:1: D205 1 blank line required between summary line and description
phys2denoise/metrics/chest_belt.py:6:1: D210 No whitespaces allowed surrounding docstring text
phys2denoise/metrics/chest_belt.py:6:1: D400 First line should end with a period

I'll still need a couple of days before I can do an in-depth review, but maybe one of the other physiopy folks will be able to get to it before me.

[tsalo]

I have a few requested changes, as well as a couple of questions that need @smoia's input.

[smoia]

Ooooooh fresh mea... I mean, members!
Welcome @rzlim08, and thank you for the PR!
I don't have time to go through it right now, but I'll start addressing @tsalo's doubts in the comments!

[tsalo]

@rzlim08 do you know if the RVT code is functional? I have an analysis where I need to use RVT and I'd like to use your function.

[rzlim08]

@rzlim08 do you know if the RVT code is functional? I have an analysis where I need to use RVT and I'd like to use your function.

Sorry for the late reply, adjusting to a job change currently. The RVT code should be functional, I think the remaining thing in this PR is to resolve some annoying merge conflicts. I would look through and make sure you agree with the choices made, I believe I modelled this off the AFNI RVT algorithm, which makes some pretty opinionated choices.

[tsalo]

Thanks @rzlim08!

[codecov]

Codecov Report

Merging #21 (24515ca) into master (14e11bb) will increase coverage by 3.51%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #21      +/-   ##
==========================================
+ Coverage   43.52%   47.04%   +3.51%     
==========================================
  Files           7        7              
  Lines         301      321      +20     
==========================================
+ Hits          131      151      +20     
  Misses        170      170              

Impacted Files	Coverage Δ
phys2denoise/metrics/chest_belt.py	85.54% <100.00%> (+4.58%)	⬆️

[kristinazvolanek]

Hi all, we made a few changes to resolve merge conflicts and test failures. Thanks for the work on this @rzlim08 and @tsalo!