-
-
Notifications
You must be signed in to change notification settings - Fork 11
round glucose values via "round-to-even" before classifying #871
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
base: master
Are you sure you want to change the base?
Changes from 2 commits
fc16e8c
291c31d
0e055a6
ef35b13
c1ed79d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,12 @@ | ||
| package glucose | ||
|
|
||
| import ( | ||
| "math" | ||
|
|
||
| "github.com/tidepool-org/platform/data" | ||
| dataBloodGlucose "github.com/tidepool-org/platform/data/blood/glucose" | ||
| "github.com/tidepool-org/platform/data/types/blood" | ||
| "github.com/tidepool-org/platform/errors" | ||
| "github.com/tidepool-org/platform/structure" | ||
| ) | ||
|
|
||
|
|
@@ -33,3 +36,75 @@ func (g *Glucose) Normalize(normalizer data.Normalizer) { | |
| g.Value = dataBloodGlucose.NormalizeValueForUnits(g.Value, units) | ||
| } | ||
| } | ||
|
|
||
| func roundToEvenWithDecimalPlaces(v float64, decimals int) float64 { | ||
| if decimals < 1 { | ||
| return math.RoundToEven(v) | ||
| } | ||
| coef := math.Pow(10, float64(decimals)) | ||
ewollesen marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| return math.RoundToEven(v*coef) / coef | ||
| } | ||
|
|
||
| type Classification string | ||
|
|
||
| const ( | ||
| ClassificationInvalid Classification = "invalid" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we get rid of this edge case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope. Go's type system isn't expressive enough. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ...unless you have a secret you're not sharing? 🤔 |
||
|
|
||
| VeryLow = "very low" | ||
| Low = "low" | ||
| OnTarget = "on target" | ||
darinkrauss marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| High = "high" | ||
| VeryHigh = "very high" | ||
| ExtremelyHigh = "extremely high" | ||
| ) | ||
|
|
||
| type classificationThreshold struct { | ||
| Name Classification | ||
| Value float64 | ||
| Inclusive bool | ||
| } | ||
|
|
||
| type Classifier []classificationThreshold | ||
|
|
||
| func (c Classifier) Classify(g *Glucose) (Classification, error) { | ||
| normalized, err := dataBloodGlucose.NormalizeValueForUnitsSafer(g.Value, g.Units) | ||
| if err != nil { | ||
| return ClassificationInvalid, errors.Wrap(err, "unable to classify") | ||
| } | ||
| // Rounded values are used for all classifications. To not do so risks introducing | ||
| // inconsistency between frontend, backend, and/or other reports. See BACK-3800 for | ||
| // details. | ||
| rounded := roundToEvenWithDecimalPlaces(normalized, 1) | ||
darinkrauss marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| for _, threshold := range c { | ||
| if threshold.Includes(rounded) { | ||
ewollesen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return threshold.Name, nil | ||
| } | ||
| } | ||
| // Ensure your highest threshold has a value like math.MaxFloat64 to avoid this. | ||
| return ClassificationInvalid, errors.Newf("unable to classify value: %v", *g) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To remove this edge case (and remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I considered that, but then there's no way to intentionally return an error if a value is too high (out of range). Trade-offs. I don't know how/when either is desirable, but it seemed easy enough to leave the capability, and there are sharp edges either way (hence the comment, warning of sharp edges). They can be removed later if that seems more appropriate. |
||
| } | ||
|
|
||
| // TidepoolADAClassificationThresholdsMmolL for classifying glucose values. | ||
| // | ||
| // All values are normalized to MmolL before classification. | ||
| // | ||
| // In addition to the standard ADA ranges, the Tidepool-specifiic "extremely high" range is | ||
| // added. | ||
| // | ||
| // It is the author's responsibility to ensure the thresholds remain sorted from smallest to | ||
| // largest. | ||
| var TidepoolADAClassificationThresholdsMmolL = Classifier([]classificationThreshold{ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, including the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, and if a map doesn't include a classification (e.g. 'Low') skip it and go to the next one (e.g. 'In Range'). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll take a look at what I can do. Using a map doesn't prevent errors (the user could miss a key, or leave a key out of the explicit ordering), and I don't wanna overcomplicate it too much. If someone doesn't read the comment and misses the fact that it's using an ordered data structure and ignores all the unit tests and gets the change past code review... Forgive me, but that's on them, don't you think? There are limits to what I can prevent. I feel like I've taken reasonable steps. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've applied a sort to the thresholds. I dunno that its worth it, but... it's... something. Let me know what you think. |
||
| {Name: VeryLow, Value: 3, Inclusive: false}, // Source: https://doi.org/10.2337/dc24-S006 | ||
| {Name: Low, Value: 3.9, Inclusive: false}, // Source: https://doi.org/10.2337/dc24-S006 | ||
| {Name: OnTarget, Value: 10, Inclusive: true}, // Source: https://doi.org/10.2337/dc24-S006 | ||
|
||
| {Name: High, Value: 13.9, Inclusive: true}, // Source: https://doi.org/10.2337/dc24-S006 | ||
| {Name: VeryHigh, Value: 19.4, Inclusive: true}, // Source: https://doi.org/10.2337/dc24-S006 | ||
| {Name: ExtremelyHigh, Value: math.MaxFloat64, Inclusive: true}, // Source: BACK-2963 | ||
| }) | ||
|
|
||
| func (c classificationThreshold) Includes(value float64) bool { | ||
| if c.Inclusive && value <= c.Value { | ||
| return true | ||
| } | ||
| return value < c.Value | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,6 @@ package types | |
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "math" | ||
| "strconv" | ||
|
|
@@ -14,6 +13,7 @@ import ( | |
| "github.com/tidepool-org/platform/data/blood/glucose" | ||
| glucoseDatum "github.com/tidepool-org/platform/data/types/blood/glucose" | ||
| "github.com/tidepool-org/platform/data/types/blood/glucose/continuous" | ||
| "github.com/tidepool-org/platform/errors" | ||
| ) | ||
|
|
||
| const MaxRecordsPerBucket = 60 // one per minute max | ||
|
|
@@ -162,31 +162,38 @@ func (rs *GlucoseRanges) Finalize(days int) { | |
| } | ||
| } | ||
|
|
||
| func (rs *GlucoseRanges) Update(record *glucoseDatum.Glucose) { | ||
| normalizedValue := *glucose.NormalizeValueForUnits(record.Value, record.Units) | ||
| func (rs *GlucoseRanges) Update(record *glucoseDatum.Glucose) error { | ||
| classification, err := glucoseDatum.TidepoolADAClassificationThresholdsMmolL.Classify(record) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you need to replace the "config" values in the summary with the new thresholds There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My fix for this is somewhat ... awkward maybe? I welcome other ideas, but I think this is going to all be changing soonish with the introduction of alternate ranges, so I'm not sure it's worth spending a ton of time on. |
||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| rs.Total.UpdateTotal(record) | ||
|
|
||
| if normalizedValue < veryLowBloodGlucose { | ||
| switch classification { | ||
| case glucoseDatum.VeryLow: | ||
| rs.VeryLow.Update(record) | ||
| rs.AnyLow.Update(record) | ||
| } else if normalizedValue > veryHighBloodGlucose { | ||
| rs.VeryHigh.Update(record) | ||
| rs.AnyHigh.Update(record) | ||
|
|
||
| // VeryHigh is inclusive of extreme high, this is intentional | ||
| if normalizedValue >= extremeHighBloodGlucose { | ||
| rs.ExtremeHigh.Update(record) | ||
| } | ||
| } else if normalizedValue < lowBloodGlucose { | ||
| case glucoseDatum.Low: | ||
| rs.Low.Update(record) | ||
| rs.AnyLow.Update(record) | ||
| } else if normalizedValue > highBloodGlucose { | ||
| rs.AnyHigh.Update(record) | ||
| rs.High.Update(record) | ||
| } else { | ||
| case glucoseDatum.OnTarget: | ||
| rs.Target.Update(record) | ||
| case glucoseDatum.High: | ||
| rs.High.Update(record) | ||
| rs.AnyHigh.Update(record) | ||
| case glucoseDatum.VeryHigh: | ||
| rs.VeryHigh.Update(record) | ||
| rs.AnyHigh.Update(record) | ||
| case glucoseDatum.ExtremelyHigh: | ||
| rs.ExtremeHigh.Update(record) | ||
| rs.VeryHigh.Update(record) | ||
| rs.AnyHigh.Update(record) | ||
| default: | ||
| errMsg := "WARNING: unhandled classification %v; THIS SHOULD NEVER OCCUR" | ||
| return errors.Newf(errMsg, classification) | ||
| } | ||
|
|
||
| rs.Total.UpdateTotal(record) | ||
| return nil | ||
| } | ||
|
|
||
| func (rs *GlucoseRanges) CalculateDelta(current, previous *GlucoseRanges) { | ||
|
|
@@ -237,7 +244,9 @@ func (b *GlucoseBucket) Update(r data.Datum, lastData *time.Time) (bool, error) | |
| return false, nil | ||
| } | ||
|
|
||
| b.GlucoseRanges.Update(record) | ||
| if err := b.GlucoseRanges.Update(record); err != nil { | ||
| return false, err | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean that if a single value has an error then it will cause the entire summary to fail? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
A good question. does quick checking At the moment, yes. However, the only possible errors are unhandled classifications or failures to normalize a value, i.e. coding errors. Coding errors typically fall into the "fail fast and early" category for me, so I feel like this is an appropriate response. Let me know if there's a different convention. I fear that a simple log and forget would lead to bad reports and we might not know why... Though I'd be surprised if there were ever an error raised from this code that wasn't caught via a unit test or QA. |
||
| } | ||
| b.LastRecordDuration = GetDuration(record) | ||
|
|
||
| return true, nil | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.