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

Fix ms hrv rsa + proposed fix for RSA P2T #766

Merged
merged 4 commits into from
Jan 31, 2023

Conversation

vivile42
Copy link
Contributor

@vivile42 vivile42 commented Jan 25, 2023

Convert RSA P2T output in ms

Description

In the description it says it should return ms but it was in second i've fixed it as discussed in here

I've also reworked the p2t to compute the variation between the max RRI in exh and the min RRI in inh following the paper from Grossman 1990

This is a working version (on the data i've tested at least ) but should just be a starting point since it's not really validated i hope it can be helpful :)

Proposed Changes

  • modified the _hrv_rsa_p2t function multiplying by 1000 the difference in TF after it has been divided by the sampling rate
  • added cycle inh and exh separately to compute minima
  • added buffer of 750 ms to the peak and end cycle in order to take into consideration end of following RRI (see Grossman 1990) to me it makes sense but i don't know if it's still done like that ( i couldn't find recent code to double check)
  • when max RRI exh - min RRI inh is negative the breathing cycle is counted as zero (again same procedure from Grossman 1990)
  • important: in that paper they take into account only breathing cycles in which the minima fell into inhalation and was preceded by a longer RRI interval, similarly the max has to be preceded by a a shorter interval. I don't want this for my data since it's an active condition in which people breathe fast, i don't want to have too much missing data. You should discuss whether you want to implement this part or not, maybe it can be toggled as optional ?

Checklist

Here are some things to check before creating the PR. If you encounter any issues, do let us know :)

  • I have read the CONTRIBUTING file.
  • My PR is targetted 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)

Converted in ms multiplying by 1000
- converted s to ms
- now it takes the diff between max RRI in exh and  min RRI in inh
@welcome
Copy link

welcome bot commented Jan 25, 2023

Thanks for opening this pull request! We'll make sure it's perfect before merging 🤗 force
Make sure to read the contributing guide. Also, if you think that your contribution is worthy of it, you can consider adding yourself to the Contributors list (feel free to ask us if you have any doubts).

@DominiqueMakowski
Copy link
Member

Hi @vivile42, sorry for the delay I was relocating to another country. I will merge as the check errors seems unrelated to these changes, thanks a lot!

@DominiqueMakowski DominiqueMakowski merged commit add9a80 into neuropsychology:dev Jan 31, 2023
@welcome
Copy link

welcome bot commented Jan 31, 2023

landing
Congrats on merging your first pull request! 🎉🍾 We're looking forward to your next one!

@DominiqueMakowski
Copy link
Member

DominiqueMakowski commented Jan 31, 2023

in that paper they take into account only breathing cycles in which the minima fell into inhalation and was preceded by a longer RRI interval, similarly the max has to be preceded by a a shorter interval. I don't want this for my data since it's an active condition in which people breathe fast, i don't want to have too much missing data. You should discuss whether you want to implement this part or not, maybe it can be toggled as optional ?

Interesting, yes we could add an argument to toggle that on or off. Feel free to open a new PR (so sorry I should have waited instead of merging this one!).

(and consider adding yourself to the contributors)

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.

2 participants