Skip to content

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Jun 28, 2021

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!

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

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

Labels

None yet

Projects

Status: PR needs review
Status: PR to review

Development

Successfully merging this pull request may close these issues.

Support negative lags in RV calculation

2 participants