[BACK-3651] Add Min and Max to glucose stats #837
Conversation
Roukoswarf
force-pushed
the
alex-back-3479-split-summary-tasks
branch
from
19f1b1f to
71d5c09
Compare
Roukoswarf
changed the base branch from
alex-back-3479-split-summary-tasks
to
master
Roukoswarf
force-pushed
the
alex-back-3651-minmax
branch
from
cdf94ba to
847cc20
Compare
| dataSetData := make([]data.Datum, requiredRecords) | ||
| uploadId := test.RandomSetID() | ||
| deviceId := "SummaryTestDevice" | ||
| // func NewDataSetCGMVariance(startTime time.Time, hours int, perHour int, StandardDeviation float64) ([]data.Datum, float64) { |
There was a problem hiding this comment.
Probably best to remove this.
| n1 := 0.0 | ||
| n2 := 0.0 | ||
| if r.Minutes > 0 { | ||
| n1 = float64(r.Minutes) | ||
| n2 = float64(new.Minutes) | ||
| } else { | ||
| n1 = float64(r.Records) | ||
| n2 = float64(new.Records) | ||
| } | ||
|
|
There was a problem hiding this comment.
| n1 := 0.0 | |
| n2 := 0.0 | |
| if r.Minutes > 0 { | |
| n1 = float64(r.Minutes) | |
| n2 = float64(new.Minutes) | |
| } else { | |
| n1 = float64(r.Records) | |
| n2 = float64(new.Records) | |
| } | |
| n1 := float64(r.Records) | |
| n2 := float64(new.Records) | |
| if r.Minutes > 0 { | |
| n1 = float64(r.Minutes) | |
| n2 = float64(new.Minutes) | |
| } | |
| func PopStdDev(x []float64) float64 { | ||
| variance := calcVariance(x, Mean(x)) / float64(len(x)) | ||
| return math.Sqrt(variance) | ||
| func PopStdDev(x []float64) (float64, float64) { |
There was a problem hiding this comment.
This name could be better now that it returns both the StdDev & the Variance too.
| } | ||
| } | ||
|
|
||
| func (m *MinMax) CalculateDelta(current *MinMax, previous *MinMax) { |
There was a problem hiding this comment.
I have lots of thoughts on this, but I think it boils down to the fact that delta values are not min/max values, and should not be stored in a type called MinMax, nor should they be stored in a struct member called MinMax. It's very confusing and will likely cause bugs in the future.
It seems more appropriate to me if there were a function like this:
func CalcDeltaMinAndDeltaMax(values ...MinMax) (deltaMin, deltaMax float64)It shouldn't be a method on MinMax, as the values it calculates are not min and max values, but rather delta values.
| @@ -245,6 +300,8 @@ func (b *GlucoseBucket) Update(r data.Datum, lastData *time.Time) (bool, error) | |||
|
|
|||
| type GlucosePeriod struct { | |||
| GlucoseRanges `json:",inline" bson:",inline"` | |||
| MinMax `json:",inline" bson:",inline"` | |||
There was a problem hiding this comment.
Naming this MinMax is very confusing to me, since it seems to be holding delta values (see line 323). Am I not understanding something?
| @@ -263,6 +320,7 @@ type GlucosePeriod struct { | |||
|
|
|||
| func (p *GlucosePeriod) CalculateDelta(current *GlucosePeriod, previous *GlucosePeriod) { | |||
| p.GlucoseRanges.CalculateDelta(¤t.GlucoseRanges, &previous.GlucoseRanges) | |||
| p.MinMax.CalculateDelta(¤t.MinMax, &previous.MinMax) | |||
There was a problem hiding this comment.
p.MinMax holds min and max values... Until CalculateDelta is called, then it holds delta of min and delta of max values. But there's nothing to stop someone from calling Update again, which would then pretend that MinMax is holding min and max values (instead of deltas), and that would be bad and completely non-obvious given the naming.
p.DeltaMin, p.DeltaMax = CalculateDeltaMinAndDeltaMax(current.MinMax, previous.MinMax)Seems like a better way to handle this.
| @@ -322,7 +380,8 @@ func (p *GlucosePeriod) Update(bucket *Bucket[*GlucoseBucket, GlucoseBucket]) er | |||
| } | |||
| } | |||
|
|
|||
| p.Add(&bucket.Data.GlucoseRanges) | |||
| p.MinMax.Add(&bucket.Data.MinMax) | |||
There was a problem hiding this comment.
This should not be allowed after a call to CalculateDelta, as the values stored in p.MinMax would at that time be delta values.
| expectedMax = k | ||
| } | ||
| if k < float64(expectedMin) { | ||
| expectedMin = k |
There was a problem hiding this comment.
I don't love that in order to test a min function, the test re-implements a min function.
If testing the min function is handled elsewhere, and I think it is, then this test isn't necessary anyway. Is there some good reason to test it here as well?
Note for deployment migrations: There is no change affecting Continuous summaries, therefore they should be moved to
schemaVersion:6during deployment.