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

Add RVT algorithm (Birn et al. 2006) #21

Merged
merged 15 commits into from
Dec 1, 2022
Merged

Conversation

rzlim08
Copy link
Contributor

@rzlim08 rzlim08 commented Dec 26, 2020

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
Copy link
Member

tsalo commented Dec 29, 2020

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
Copy link
Contributor Author

rzlim08 commented Jan 3, 2021

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
Copy link
Member

tsalo commented Jan 4, 2021

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.

Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

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

.gitignore Outdated Show resolved Hide resolved
phys2denoise/metrics/chest_belt.py Show resolved Hide resolved
phys2denoise/metrics/chest_belt.py Outdated Show resolved Hide resolved
phys2denoise/tests/test_rvt.py Outdated Show resolved Hide resolved
phys2denoise/tests/test_rvt.py Show resolved Hide resolved
@smoia
Copy link
Member

smoia commented Jan 7, 2021

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!

@smoia smoia added the Minormod This PR generally closes an "Enhancement" issue. It increments the minor version (0.+1.0) label Jan 7, 2021
@smoia smoia changed the title add RVT draft algorithm Add RVT algorithm Jan 7, 2021
@smoia smoia changed the title Add RVT algorithm Add RVT algorithm (Birn et al. 2006) Jan 7, 2021
@tsalo tsalo mentioned this pull request Feb 25, 2021
18 tasks
@tsalo
Copy link
Member

tsalo commented Aug 10, 2021

@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
Copy link
Contributor Author

rzlim08 commented Aug 18, 2021

@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
Copy link
Member

tsalo commented Aug 18, 2021

Thanks @rzlim08!

@kristinazvolanek kristinazvolanek self-assigned this Nov 29, 2022
@codecov
Copy link

codecov bot commented Dec 1, 2022

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

Impacted file tree graph

@@            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 kristinazvolanek merged commit 709f3fd into physiopy:master Dec 1, 2022
@kristinazvolanek
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Minormod This PR generally closes an "Enhancement" issue. It increments the minor version (0.+1.0)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Requested metric: respiratory-volume-per-time
4 participants