carekit-apple / CareKit

CareKit is an open source software framework for creating apps that help people better understand and manage their health.
https://www.researchandcare.org
Other
2.41k stars 444 forks source link

feat: Allow developers to create instances of LinearCareTaskProgress and BinaryCareTaskProgress #713

Closed cbaker6 closed 5 months ago

cbaker6 commented 5 months ago

Making the initializer public allows developers create linear progress, similar to https://github.com/carekit-apple/CareKit/blob/be7749c23c0601a81d5aaf5d169b51b1d1e4b80b/CareKitStore/CareKitStore/TaskProgress/AggregatedCareTaskProgress.swift#L96-L110

gavirawson-apple commented 5 months ago

Hey Corey, do you have a use case in mind where it's useful to create these progress structs manually? At the moment, the entry point is a helper method on a CareKit event:

// Existing entry point
let progress = event.computeProgress(by: .checkingOutcomeExists)

// Internal entry point. In what scenarios is this useful as public API?
let progress = BinaryCareTaskProgress(isCompleted: true)
cbaker6 commented 5 months ago

Hey @gavirawson-apple, I don't necessarily have a use case for BinaryCareTaskProgress and can get away with using event.computeProgress(by: .checkingOutcomeExists) as you mentioned.

When it comes to LinearCareTaskProgress, I was attempting to convert code in one of our apps from the previous version of CareKit that used custom OCKEventAggregator's to compute the mean and median progress. For the conversion, I was attempting to take advantage of creating LinearCareTaskProgress. Currently, I copy/pasted LinearCareTaskProgress and renamed it to TempLinearCareTaskProgress to be able to create instances.

This allows me to do something like:

extension CareTaskProgressStrategy {

    static func computeProgressByCalculatingMeanOutcomeValues(for event: OCKAnyEvent) -> TempLinearCareTaskProgress {

        let outcomeValues = event.outcome?.values ?? []

            let numberOfCompletedOutcomes = Double(outcomeValues.count)
            let summedOutcomesValue = outcomeValues
                .map(accumulableDoubleValue)
                .reduce(0, +)

            let targetValues = event.scheduleEvent.element.targetValues

            let summedTargetValue = targetValues
                .map(accumulableDoubleValue)
                .reduce(nil) { partialResult, nextTarget -> Double? in
                    return sum(partialResult, nextTarget)
                }

            if numberOfCompletedOutcomes == 0 {
                return TempLinearCareTaskProgress(value: 0, goal: summedTargetValue)
            } else {
                let mean = summedOutcomesValue / numberOfCompletedOutcomes
                return TempLinearCareTaskProgress(value: summedOutcomesValue, goal: summedTargetValue)
            }
    }
}

Let me know if you suggest I approach this differently.

gavirawson-apple commented 5 months ago

Thanks for the snippet, seems worthwhile to open the initializer up if it's helpful. The original thinking was that clients can create a custom struct that conforms to CareTaskProgress rather than using the ones in the framework, but filling out fractionCompleted probably isn't always trivial. Let's do it.

cbaker6 commented 5 months ago

The original thinking was that clients can create a custom struct that conforms to CareTaskProgress rather than using the ones in the framework, but filling out fractionCompleted probably isn't always trivial

In addition, developers will need to create an instance of LinearCareTaskProgress for creating OCKDataSeriesConfiguration and OCKCartesianChartViewController, though this probably won't be needed when switching to Swift Charts: https://github.com/carekit-apple/CareKit/blob/fbdfa3a14ad9a002b67e8484f344e92f2581629b/CareKit/CareKit/iOS/Chart/View%20Controllers/OCKDataSeriesConfiguration.swift#L84-L110

gavirawson-apple commented 5 months ago

@cbaker6 I see there are some failing tests on CI. Are tests passing locally for you?

cbaker6 commented 5 months ago

Hey @gavirawson-apple I've run all of the test locally and they pass. I haven't been able to replicate the failures the CI finds

gavirawson-apple commented 5 months ago

Gotcha, passes locally for me too so it looks good to go. We can merge the PR and I'll make a note to look into the CI issues.