Skip to content

WIP first stab at new bucketing using rounded mg/dL #910

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.

Sign up for GitHub

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

Draft
ewollesen wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

WIP first stab at new bucketing using rounded mg/dL #910

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
86820e8
WIP first stab at new bucketing using rounded mg/dL
ewollesen Jan 29, 2026
3ec2291
multiple changes from code review
ewollesen Jan 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions data/blood/glucose/glucose.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
package glucose

import (
"fmt"
"math"
"slices"
"strings"

"github.com/tidepool-org/platform/pointer"
)
Expand Down Expand Up @@ -82,6 +85,34 @@ func NormalizeValueForUnits(value *float64, units *string) *float64 {
return value
}

func ConvertValue(value float64, fromUnits, toUnits string) (float64, error) {

@toddkazakov toddkazakov Jan 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a rather complicated way to implement a generic function that will always be restrained to one of two operations mg/dL -> mmol/l or mmol\l -> mg/dL. I suggest defining two functions instead and using those directly:

func MgdLToMmolL(value float64) float64 {
	rounded := int(value/MmolLToMgdLConversionFactor*MmolLToMgdLPrecisionFactor + math.Copysign(0.5, value))
	return float64(rounded) / MmolLToMgdLPrecisionFactor
}

func MmolLToMgdL(value float64) float64 {
	return value * MmolLToMgdLConversionFactor
}

@ewollesen ewollesen Jan 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a rather complicated way to implement a generic function

Agreed. I took a slightly different approach, but hopefully it captures your intent.

if fromUnits == toUnits {
return value, nil
}
units := Units()
if !slices.Contains(units, fromUnits) {
return 0, fmt.Errorf("unrecognized from units %q not found in %q", fromUnits, units)
}
if !slices.Contains(units, toUnits) {
return 0, fmt.Errorf("unrecognized to units %q not found in %q", fromUnits, units)
}

switch strings.ToLower(fromUnits + toUnits) {
case strings.ToLower(Mgdl + MmolL):
v := NormalizeValueForUnits(&value, &fromUnits)
// NormalizeValueForUnits will return the original value if the from units aren't
// recognized.
if *v == value {
return 0, fmt.Errorf("unhandled from units: %q", fromUnits)
}
return *v, nil
case strings.ToLower(MmolL + MgdL):
return value * MmolLToMgdLConversionFactor, nil
default:
return 0, fmt.Errorf("unhandled from units: %q", fromUnits)
}
}

func ValueRangeForRateUnits(rateUnits *string) (float64, float64) {
if rateUnits != nil {
switch *rateUnits {
Expand Down
58 changes: 51 additions & 7 deletions summary/types/glucose.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ package types

import (
"context"
"errors"
"fmt"
"log/slog"
"math"
"strconv"
"strings"
Expand All @@ -15,6 +15,7 @@ import (
"github.com/tidepool-org/platform/data/blood/glucose"
"github.com/tidepool-org/platform/data/types/blood/glucose/continuous"
"github.com/tidepool-org/platform/data/types/blood/glucose/selfmonitored"
"github.com/tidepool-org/platform/errors"
)

const (
Expand All @@ -24,6 +25,7 @@ const (

type Glucose interface {
NormalizedValue() float64
RawValueAndUnits() (float64, string, error)

@toddkazakov toddkazakov Jan 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you expose the underlying value and the underlying units functions in the adapters directly instead of combining them?

Type() string
Time() *time.Time
CreatedTime() *time.Time
Expand All @@ -49,6 +51,17 @@ type SelfMonitoredGlucoseAdapter struct {
datum *selfmonitored.SelfMonitored
}

func (s SelfMonitoredGlucoseAdapter) RawValueAndUnits() (float64, string, error) {
if s.datum == nil {
return 0, "", errors.New("datum is nil")
} else if s.datum.Value == nil {
return 0, "", errors.New("datum's value is nil")
} else if s.datum.Units == nil {
return 0, "", errors.New("datum's units are nil")
}
return *s.datum.Value, *s.datum.Units, nil
}

func (s SelfMonitoredGlucoseAdapter) NormalizedValue() float64 {
return *glucose.NormalizeValueForUnits(s.datum.Value, s.datum.Units)
}
Expand All @@ -75,6 +88,17 @@ type ContinuousGlucoseAdapter struct {
datum *continuous.Continuous
}

func (c ContinuousGlucoseAdapter) RawValueAndUnits() (float64, string, error) {
if c.datum == nil {

@toddkazakov toddkazakov Jan 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a little too defensive? The NormalizedValue() dereferences the return value pointer - i.e. it assumes that the glucose datum is valid (it should be, as it was persisted in the database) and that value and units are not nil.

@ewollesen ewollesen Jan 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a little too defensive? The NormalizedValue() dereferences the return value pointer - i.e. it assumes that the glucose datum is valid (it should be, as it was persisted in the database) and that value and units are not nil.

My experience tells me assumptions about things like this will come back to bite us, with difficult to identify bugs or errors. However, as you noted, it is far more defensive than the existing related code, making it awkward as a result. So I'll adjust it to align better with what's there presently.

@toddkazakov toddkazakov Jan 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This particular code path hasn't blown up in the past 2-3 years, so it's pretty safe

return 0, "", errors.New("datum is nil")
} else if c.datum.Value == nil {
return 0, "", errors.New("datum value is nil")
} else if c.datum.Units == nil {
return 0, "", errors.New("datum units are nil")
}
return *c.datum.Value, *c.datum.Units, nil
}

func (c ContinuousGlucoseAdapter) NormalizedValue() float64 {
return *glucose.NormalizeValueForUnits(c.datum.Value, c.datum.Units)
}
Expand Down Expand Up @@ -262,24 +286,44 @@ func (rs *GlucoseRanges) Finalize(days int) {
}
}

const (

@toddkazakov toddkazakov Jan 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you noted, we should update the summary config to include those new thresholds. I'd recommend keeping the threshold units consistent with the normalized glucose units already used in the summary, specifically, converting those integer values to mmol/L. As far as I know, we don't store glucose values or thresholds in mg/dL in the database, so it's better not to introduce a precedent.

veryLowBloodGlucoseMgdL int = 54
lowBloodGlucoseMgdL int = 70
highBloodGlucoseMgdL int = 180
veryHighBloodGlucoseMgdL int = 250
extremeHighBloodGlucoseMgdL int = 350
)

func (rs *GlucoseRanges) Update(record Glucose) {
normalizedValue := record.NormalizedValue()
rawValue, rawUnits, err := record.RawValueAndUnits()

@toddkazakov toddkazakov Jan 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can probably be less defensive here - if we are here we should have a value and units and those should not be nil. If you really need to log something, I suggest you change the signature of the function and return an error, instead of passing a logger. Just let the caller log it.

if err != nil {
// TODO pass in a proper platform logger
slog.Error("unable to update datum", "error", err)
return
}
mgdlValue, err := glucose.ConvertValue(rawValue, rawUnits, glucose.MgdL)
if err != nil {
// TODO pass in a proper platform logger
slog.Error("unable to update datum: conversion error", "error", err)
return
}
mgdlRounded := int(math.Round(mgdlValue))

if normalizedValue < veryLowBloodGlucose {
if mgdlRounded < veryLowBloodGlucoseMgdL {
rs.VeryLow.Update(record)
rs.AnyLow.Update(record)
} else if normalizedValue > veryHighBloodGlucose {
} else if mgdlRounded > veryHighBloodGlucoseMgdL {
rs.VeryHigh.Update(record)
rs.AnyHigh.Update(record)

// VeryHigh is inclusive of extreme high, this is intentional
if normalizedValue >= extremeHighBloodGlucose {
if mgdlRounded >= extremeHighBloodGlucoseMgdL {
rs.ExtremeHigh.Update(record)
}
} else if normalizedValue < lowBloodGlucose {
} else if mgdlRounded < lowBloodGlucoseMgdL {
rs.Low.Update(record)
rs.AnyLow.Update(record)
} else if normalizedValue > highBloodGlucose {
} else if mgdlRounded > highBloodGlucoseMgdL {
rs.AnyHigh.Update(record)
rs.High.Update(record)
} else {
Expand Down