Migrate spezi health kit#3
Conversation
| CODE_SIGN_STYLE = Automatic; | ||
| CURRENT_PROJECT_VERSION = 1; | ||
| DEVELOPMENT_ASSET_PATHS = "\"HealthyLLM/Supporting Files/Preview Content\""; | ||
| DEVELOPMENT_TEAM = CQRZ4E7K9U; |
There was a problem hiding this comment.
I will revert the development team and bundle identifier after addressing all other PR comments
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/add-healthyllm #3 +/- ##
=========================================================
- Coverage 4.43% 4.29% -0.13%
=========================================================
Files 70 69 -1
Lines 633 630 -3
=========================================================
- Hits 28 27 -1
+ Misses 605 603 -2
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
|
||
| // One-off query for aggregating health data | ||
| // TODO: Create Pull Request for SpeziHealthKit | ||
| extension HealthKit { | ||
| public func statisticsQuery<Sample>( |
There was a problem hiding this comment.
@lukaskollmer do you know if SpeziHealthKit already provides the functionality for one-off statistics queries in some way? If not, and you think it would be useful to others as well, I'm happy to create a PR!
There was a problem hiding this comment.
i don't think so; iirc i was planning to create a nice API around HKStatistics (eg so that you dont have to deal with the optional HKQuantity objects that will never be nil) but i didn't get around to it. we do have the @HealthKitStatisticsQuery for SwiftUI; would make sense to add a non-SwiftUI counterpart for one-off queries. we might even be able to reuse the code internally
There was a problem hiding this comment.
the @HealthKitStatisticsQuery API already contains an initial attempt at making this type safe; we have different init overloads to prevent the caller from passing in invalid aggregation option combinations
There was a problem hiding this comment.
@lukaskollmer and I had a small discussion on Monday about a very simple approach to make the SwiftUI property wrappers also available outside of the SwiftUI hierarchy and usable inside modules, we might want to tackle this in the future, IMO that would solve the issue here as well. But we might want merge this first and then iterate on it later.
There was a problem hiding this comment.
Sounds good, @lukaskollmer let me know if you need some support, but for now I agree with merging first and iterating later 👍
There was a problem hiding this comment.
@max-rosenblattl Would be great if you could create an issue in SpeziHealthKit that tracks this issue. I also saw that @phnagy was looking into some issues there; maybe that might be a great first issue with a real need and direct implementation pathway. (CC @lukaskollmer)
There was a problem hiding this comment.
@lukaskollmer I phrased the issue in a way that suggests implementing your discussed approach; if you have any, it would be great to add some specific details from your discussion that could ease the implementation.
| try? await self.fetchSample( | ||
| for: identifier, | ||
| unit: unit, | ||
| startDate: endDate, | ||
| endDate: .now | ||
| ) |
There was a problem hiding this comment.
Currently, we’re aggregating time-series data into a single value (e.g., average heart rate over the last month). It might be more meaningful to use a different aggregation approach; for example, retrieving daily averages over the last month instead of just one number. This would also align better with future EmbedHealth integration, even if that’s a larger effort. Let me know what you think @PSchmiedmayer @LeonNissen @lukaskollmer
There was a problem hiding this comment.
agree that a higher granularity would be desirable but wasn't the issue here that having too many samples could exceed the context window? (i might be confusing this with something else).
iirc another idea of mine at the time was that we could use different granularities depending on how far back the data was collected (eg weekly heart rate for > a year ago, daily average for > last week, and hourly averages for the past 7 days)
There was a problem hiding this comment.
Yes, the main issue was the growing context window; that's why we constrained it to a few samples.
Agree with @lukaskollmer here for this approach. I think 7 days for a week, weekly samples for a month, or monthly samples for a year would make sense and don't dramatically exceed the context window. Would be good to verify this with some on-device testing @max-rosenblattl before we merge this.
There was a problem hiding this comment.
Thanks for the insights! I think it makes a lot of sense, happy to tackle that.
Regarding testing on device: I will be able to do so in the very near future; until then if anyone has some spare minutes, I would appreciate it!
There was a problem hiding this comment.
Quick update: finally got my new iPhone and could test it on device (results in PR description above)! Also adapted the statistics query to handle the discussed time-range based aggregation intervals. Just saw they were integrated in here too StanfordSpezi/SpeziHealthKit#65 , quite cool looking forward to the merge 🚀
There was a problem hiding this comment.
Thank you for working on all of this @max-rosenblattl 🚀
There was a problem hiding this comment.
Thank you for working on this and thank you for your initial comments @lukaskollmer 👍
Happy to see a first version merged once the CI is passing; we will have a decent amount of iterations here when we will embed the time series LLM components but that is a next step after that.
| try? await self.fetchSample( | ||
| for: identifier, | ||
| unit: unit, | ||
| startDate: endDate, | ||
| endDate: .now | ||
| ) |
There was a problem hiding this comment.
Yes, the main issue was the growing context window; that's why we constrained it to a few samples.
Agree with @lukaskollmer here for this approach. I think 7 days for a week, weekly samples for a month, or monthly samples for a year would make sense and don't dramatically exceed the context window. Would be good to verify this with some on-device testing @max-rosenblattl before we merge this.
|
|
||
| // One-off query for aggregating health data | ||
| // TODO: Create Pull Request for SpeziHealthKit | ||
| extension HealthKit { | ||
| public func statisticsQuery<Sample>( |
There was a problem hiding this comment.
@lukaskollmer and I had a small discussion on Monday about a very simple approach to make the SwiftUI property wrappers also available outside of the SwiftUI hierarchy and usable inside modules, we might want to tackle this in the future, IMO that would solve the issue here as well. But we might want merge this first and then iterate on it later.
Migrate HealthKit to SpeziHealthKit
♻️ Current situation & Problem
The current implementation doesn't take advantage of
SpeziHealthKit.⚙️ Release Notes
This PR migrates from HealthKit to SpeziHealthKit, improving readability and simplifying the access of health data.
📚 Documentation
HealthQueryshould optimally be merged into theSpeziHealthKitframework.✅ Testing
Tested on iPhone Air.
Code of Conduct & Contributing Guidelines
By creating and submitting this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: