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.

Sign up for GitHub

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

Jump to bottom

Fix lag application for negative lags #36

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Fix lag application for negative lags #36

wants to merge 7 commits into from

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Jun 28, 2021 •
edited
Loading

Closes #35.

This PR corrects the calculation of negative lags.

Proposed Changes

  • Remove unused lags parameter in rv function.
  • Use the absolute lag when applying negative lags.
  • Replace uses of numpy.int (deprecated) with int.

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 approved these changes Feb 19, 2023
View reviewed changes
Copy link
Member

@smoia smoia left a comment

Choose a reason for hiding this comment

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

LGTM!

@smoia
Copy link
Member

smoia commented Feb 19, 2023

Actually, @tsalo, it's difficult for me to figure out which changes you wanted to stay - can you resolve the conflicts with master, please?

@tsalo
Copy link
Member Author

tsalo commented Mar 15, 2023

Sorry for the delay @smoia. I'll give that a try now.

@tsalo tsalo changed the title Support negative lags in RV metric Fix lag application for negative lags Mar 15, 2023
@tsalo
Copy link
Member Author

tsalo commented Mar 15, 2023 •
edited
Loading

I ended up having to change a few things. Lags were removed from rv at some point, but the parameter was still there, so I removed the parameter.

Also, lags in apply_lags wasn't applying lags correctly, AFAICT. I think I've fixed that.

@codecov
Copy link

codecov bot commented Mar 15, 2023

Codecov Report

Merging #36 (f7c85b1) into master (24515ca) will not change coverage.
The diff coverage is 33.33%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #36   +/-   ##
=======================================
  Coverage   47.04%   47.04%           
=======================================
  Files           7        7           
  Lines         321      321           
=======================================
  Hits          151      151           
  Misses        170      170
Impacted Files Coverage Δ
phys2denoise/metrics/cardiac.py 94.73% <ø> (ø)
phys2denoise/metrics/utils.py 69.56% <0.00%> (ø)
phys2denoise/metrics/chest_belt.py 85.54% <100.00%> (ø)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smoia smoia smoia approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Physiopy
Status: PR needs review
phys2denoise
Status: PR to review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support negative lags in RV calculation
2 participants
@tsalo @smoia