StanfordBDHG / PediatricAppleWatchStudy

The Pediatric Apple Watch Study Application
MIT License
4 stars 0 forks source link

Supplemental Metrics for ECG Reading #47

Closed MatthewTurk247 closed 7 months ago

MatthewTurk247 commented 7 months ago

Supplemental Metrics for ECG Reading

:recycle: Current situation & Problem

Building on top of #40, this pull request seeks to package ECG readings with supplemental information like VO2 max and pulse rate in a way that is sensitive to the possible limitations of current recording capabilities.

:gear: Release Notes

:books: Documentation

In-line documentation will be written in relevant files in conformance to the Spezi Documentation Guide.

:white_check_mark: Testing

Tests may be added for ensuring that data is correctly serialized and properly handled.

Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

MatthewTurk247 commented 7 months ago

Some questions for @PSchmiedmayer:

codecov[bot] commented 7 months ago

Codecov Report

Attention: 101 lines in your changes are missing coverage. Please review.

Comparison is base (78f8287) 31.66% compared to head (71e7d0e) 30.33%.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/StanfordBDHG/PediatricAppleWatchStudy/pull/47/graphs/tree.svg?width=650&height=150&src=pr&token=dxs74T2g0s&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=StanfordBDHG)](https://app.codecov.io/gh/StanfordBDHG/PediatricAppleWatchStudy/pull/47?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=StanfordBDHG) ```diff @@ Coverage Diff @@ ## main #47 +/- ## ========================================== - Coverage 31.66% 30.33% -1.33% ========================================== Files 33 33 Lines 992 1118 +126 ========================================== + Hits 314 339 +25 - Misses 678 779 +101 ``` | [Files](https://app.codecov.io/gh/StanfordBDHG/PediatricAppleWatchStudy/pull/47?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=StanfordBDHG) | Coverage Δ | | |---|---|---| | [PAWS/PAWSDelegate.swift](https://app.codecov.io/gh/StanfordBDHG/PediatricAppleWatchStudy/pull/47?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=StanfordBDHG#diff-UEFXUy9QQVdTRGVsZWdhdGUuc3dpZnQ=) | `96.88% <100.00%> (+1.11%)` | :arrow_up: | | [PAWS/PAWSStandard.swift](https://app.codecov.io/gh/StanfordBDHG/PediatricAppleWatchStudy/pull/47?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=StanfordBDHG#diff-UEFXUy9QQVdTU3RhbmRhcmQuc3dpZnQ=) | `3.95% <0.00%> (-0.70%)` | :arrow_down: | | [PAWS/ECGRecordings/ECGRecording.swift](https://app.codecov.io/gh/StanfordBDHG/PediatricAppleWatchStudy/pull/47?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=StanfordBDHG#diff-UEFXUy9FQ0dSZWNvcmRpbmdzL0VDR1JlY29yZGluZy5zd2lmdA==) | `0.00% <0.00%> (ø)` | | ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/StanfordBDHG/PediatricAppleWatchStudy/pull/47?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=StanfordBDHG). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=StanfordBDHG) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/StanfordBDHG/PediatricAppleWatchStudy/pull/47?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=StanfordBDHG). Last update [78f8287...71e7d0e](https://app.codecov.io/gh/StanfordBDHG/PediatricAppleWatchStudy/pull/47?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=StanfordBDHG). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=StanfordBDHG).
PSchmiedmayer commented 7 months ago

@MatthewTurk247 Sorry for the late response!

While SpeziHealthKit is great to collect longitudinal data, I would suggest to use normal "custom" HealthKit queries for the 5 minute intervals around the measurements. You can use the Spezi Delegate HealthKit configuration with a .manual() delivery setting to make sure that permissions are granted during the onboarding but I would suggest that we develop a custom HealthKit query that gets triggered when a new ECG is recorded and received in the Standard that would then trigger the surrounding data collection.

I think it is fine to just store them in the database as they can be correlated with the ECG using the timestamps. In the long run we could work with FHIR references between the different documents but that might be too complicated for now. Just storing them as observations should be fine and should address our need.

We would also need some additional logic around the correlation of incoming symptoms and how to correlate this with the ECG types.

Let's use the meeting today to take a deep-dive into some of the implementation ideas and how we can address them as part of this and future PRs.

PSchmiedmayer commented 7 months ago

@MatthewTurk247 I would merge the PR if you agree that you don't want to make any additional changes? I can override the missing patch requirement. It will be nearly impossible to easily unit test them with the current testing capabilities that are available for HealthKit.

MatthewTurk247 commented 7 months ago

@MatthewTurk247 I would merge the PR if you agree that you don't want to make any additional changes? I can override the missing patch requirement. It will be nearly impossible to easily unit test them with the current testing capabilities that are available for HealthKit.

Yep, sounds good to me! I don't have any other changes in mind for this specific PR. So once that missing patch requirement is overridden and the PR is merged, I think it will make sense to open a new PR specifically for symptom tagging and linking to ECG recordings.

PSchmiedmayer commented 7 months ago

Sounds great; I have merged the PR 🎉

PSchmiedmayer commented 7 months ago

Thank you for all the work and research that went into the PR @MatthewTurk247 🚀