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

[feature] added PPG quality assessment functionality #987

Merged
merged 7 commits into from
May 21, 2024

Conversation

peterhcharlton
Copy link
Contributor

@peterhcharlton peterhcharlton commented May 7, 2024

Description

This PR aims at adding a new feature: PPG quality assessment. PPG quality assessment functionality has been added, and two initial quality assessment techniques have been included.

Proposed Changes

  • Added ppg_quality.py
  • Adjusted ppg_process.py to include quality assessment in the default processing pipeline
  • Added details of the PPG quality assessment methods to ppg_methods.py

Checklist

  • I have read the CONTRIBUTING file.
  • My PR is targeted at the dev branch (and not towards the master branch).
  • I ran the CODE CHECKS on the files I added or modified and fixed the errors.
  • I have added the newly added features to News.rst (if applicable)

@peterhcharlton peterhcharlton changed the title added PPG quality assessment functionality [feature] added PPG quality assessment functionality May 7, 2024
@DominiqueMakowski DominiqueMakowski requested a review from danibene May 9, 2024 13:14
Copy link
Collaborator

@danibene danibene left a comment

Choose a reason for hiding this comment

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

Hi @peterhcharlton , this is a really nice addition, thanks for your contribution! I think the tests may be failing because of the deletion of "PPG_Peaks" from the signals dataframe.

# Prepare output
signals = pd.DataFrame(
{
"PPG_Raw": ppg_signal,
"PPG_Clean": ppg_cleaned,
"PPG_Rate": rate,
"PPG_Peaks": peaks_signal["PPG_Peaks"].values,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you intentionally delete this?

Copy link
Member

Choose a reason for hiding this comment

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

This should probably not be deleted

Copy link
Collaborator

Choose a reason for hiding this comment

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

I added it back I think, not sure why it’s not showing up here: peterhcharlton@1cd76e4

Copy link
Member

Choose a reason for hiding this comment

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

Thanks Dani! Let's merge then :)

@danibene danibene merged commit 1dd967b into neuropsychology:dev May 21, 2024
8 checks passed
@peterhcharlton
Copy link
Contributor Author

My apologies for not getting back to you sooner. Thank you for your work on this. To address the changes made:

  • thanks for splitting the string to reduce the line length and splitting the text onto multiple lines.
  • regarding "PPG_Peaks" in the PPG dataframe (in ppg_process.py): I can't immediately remember why I deleted this. However, I note two things:
    • there is a function named ppg_peaks as well as this entry in the dataframe. At one point I thought perhaps some errors I was encountering might be due to having a variable and a function both named ppg_peaks. Therefore, I used ppg_pw_peaks as the variable named (e.g. in ppg_quality.py). In reality, I don't know whether I needed to avoid using the same name for both the variable and the function, or whether I could have kept to using the same name.
    • the ECG dataframe (defined in ecg_process.py) does not appear to contain an entry for ECG peaks / R-waves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants