round glucose values via "round-to-even" before classifying #871

ewollesen · 2025-06-20T18:40:53Z

The aim is to maintain consistency with the front end, and be easy to
explain when values don't align with other interpretations due to
rounding differences.

BACK-3800

The aim is to maintain consistency with the front end, and be easy to explain when values don't align with other interpretations due to rounding differences. BACK-3800

BACK-3800

toddkazakov · 2025-06-24T13:19:33Z

summary/types/glucose.go

-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)


I think you need to replace the "config" values in the summary with the new thresholds

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.

data/types/blood/glucose/glucose.go

darinkrauss

Some thoughts

data/types/blood/glucose/glucose.go

darinkrauss · 2025-06-24T18:07:19Z

data/types/blood/glucose/glucose.go

+		}
+	}
+	// Ensure your highest threshold has a value like math.MaxFloat64 to avoid this.
+	return ClassificationInvalid, errors.Newf("unable to classify value: %v", *g)


To remove this edge case (and remove the math.MaxFloat64) can we just remove the last classificationThreshold below and assume that if it isn't yet classified then in must be ExtremelyHigh?

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.

darinkrauss · 2025-06-24T18:23:18Z

summary/types/glucose.go


-	b.GlucoseRanges.Update(record)
+	if err := b.GlucoseRanges.Update(record); err != nil {
+		return false, err


Does this mean that if a single value has an error then it will cause the entire summary to fail?

Does this mean that if a single value has an error then it will cause the entire summary to fail?

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.

darinkrauss · 2025-06-24T18:35:58Z

data/types/blood/glucose/glucose.go

+//
+// It is the author's responsibility to ensure the thresholds remain sorted from smallest to
+// largest.
+var TidepoolADAClassificationThresholdsMmolL = Classifier([]classificationThreshold{


Hmm, including the Name in the classificationThreshold could lead to some future errors. For example, not realizing that the order was important, someone could reorder these and cause havoc. Consider a map and explicitly classifying in the correct order (i.e. [VeryLow, Low, InRange, High, VeryHigh] and anything else is ExtremelyHigh)?

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').

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.

I've applied a sort to the thresholds. I dunno that its worth it, but... it's... something. Let me know what you think.

darinkrauss · 2025-06-24T18:38:02Z

data/types/blood/glucose/glucose.go

+var TidepoolADAClassificationThresholdsMmolL = Classifier([]classificationThreshold{
+	{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


Will we ever have different values for Inclusive for a specific classifier or will the classification itself determine the inclusiveness. That is, is High always inclusive, or can I have a classifier where it is not?

Will we ever have different values for Inclusive for a specific classifier or will the classification itself determine the inclusiveness. That is, is High always inclusive, or can I have a classifier where it is not?

I don't know. I know there are "custom" ranges coming down the pipe, which is why I've kept the possibility.

data/types/blood/glucose/glucose.go

darinkrauss · 2025-06-24T18:43:26Z

data/types/blood/glucose/glucose.go

+type Classification string
+
+const (
+	ClassificationInvalid Classification = "invalid"


Can we get rid of this edge case?

Nope. Go's type system isn't expressive enough.

...unless you have a secret you're not sharing? 🤔

- modify roundToEventWithDecimalPlaces work for values < 0 It was never intended to be used that way, but it costs nothing to allow it. - use math.Pow10 > math.Pow(10,x) I don't think it makes a difference, but it's certainly possible that Pow10 has some optimizations, so why not. - rename "on target" => "in range" While some Tidepool docs used "target", Darin indicates that it's awkward and strange sounding, while "in range" is much more common. A quick review of some of the ADA materials correlates with Darin so ... change made. - Adjust the returned summary Config to use the threshold values actually used in the classification These used to be constants in another part of the code. Now they're pulled from the same place, so they'll hopefully not get out of sync. BACK-3800

BACK-3800

Sorts by Value ascending, then exclusive before inclusive. This should have a negligible performance impact, while providing extra safety, should someone mistakenly re-arrange the threshold classifications. One has to understand the sorting, but it doesn't even matter unless there are multiple classifications with the same Value, so that's a very small risk. BACK-3800

ewollesen force-pushed the eric-rounding branch from 68e39a4 to e8e00d6 Compare June 20, 2025 18:43

round glucose values via "round-to-even" before classifying

fc16e8c

The aim is to maintain consistency with the front end, and be easy to explain when values don't align with other interpretations due to rounding differences. BACK-3800

ewollesen force-pushed the eric-rounding branch from e8e00d6 to fc16e8c Compare June 23, 2025 23:13

adds test cases specified by product in BACK-3800

291c31d

BACK-3800

toddkazakov reviewed Jun 24, 2025

View reviewed changes

data/types/blood/glucose/glucose.go Show resolved Hide resolved

darinkrauss reviewed Jun 24, 2025

View reviewed changes

ewollesen added 3 commits June 25, 2025 13:20

tweak some comment wording

ef35b13

BACK-3800

Uh oh!

round glucose values via "round-to-even" before classifying #871

Are you sure you want to change the base?

round glucose values via "round-to-even" before classifying #871

Uh oh!

Conversation

ewollesen commented Jun 20, 2025

Uh oh!

toddkazakov Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

darinkrauss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ewollesen Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

toddkazakov Jun 24, 2025 •

edited

Loading

ewollesen Jun 25, 2025 •

edited

Loading