-
Notifications
You must be signed in to change notification settings - Fork 8
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
DRAFT PR - Add cardiac and respiratory quality metrics (from the OHBM 2024 hackathon) #44
base: master
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
@me-pic do you want to have a look at this one? |
@smoia Can try to have a look at it by the end of the month ! @bbfrederick Thanks a lot for getting the ball rolling on that 🥳 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much @bbfrederick for opening that PR ! It is a big push for the development of physioqc. Super exciting 🤩
I quickly went through the changes, and left a few comments to discuss. Will take a closer in the next week or two. Happy to help wherever I can !
return signal.sosfiltfilt(sos, inputdata).real | ||
|
||
|
||
def readbidstsv(inputfilename, colspec=None, warn=True, debug=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really happy to see physioqc supporting BIDS dataset !!! 😸
I'm wondering if there is a reason not to add pybids as a dependency, which might facilitate the loading data from a bids dataset part
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can check that out - I just pulled in a routine I wrote a while back to use in one of my packages to read BIDS files, but I can use a standard routine.
return thefit | ||
|
||
|
||
def detrend(inputdata, order=1, demean=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smoia What do you think about the idea of integrating that function in the peakdet operations module instead ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Detrend? Sure. When I went to fork peakdet, I saw that the description says "Former toolbox for physiological peak detection analyses. Deprecated in favour of prep4phys". So should I be using that instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question ! @smoia @m-miedema WDYT ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say let's keep working on peakdet and we'll project the changes to prep4phys. I'm surprised peakdet didn't have detrending options before!
return heartbeatlist | ||
|
||
|
||
def plotheartbeatqualities(heartbeatlist, totaltime=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if all the plotting functions (or just lines related to plotting in general) should not be integrated in the visualizations script ?
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Closes #
This PR adds in the quality metric code that I wrote at the OHBM 2024
Proposed Changes
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