nightscout / Trio

MIT License
46 stars 129 forks source link

Refactor and optimize `storeCarbs` function to limit carb equivalents of FPU conversion to 1.0 grams #240

Closed dnzxy closed 1 month ago

dnzxy commented 1 month ago

Contents

This pull request refactors the storeCarbs function to improve readability, maintainability, and performance. The new version introduces a helper function processFPU to handle fat and protein units (FPUs) and ensure that each carb equivalent is at least 1.0 grams. The changes also include updated docstrings for better documentation.

Solves an issue where oref0's meal.js function ignores carb entries that are less than 1.0 grams.

Changes

Detailed Changes

aug0211 commented 1 month ago

Any entry that converts to <1g carbs is rounded up to 1g.

Is this the intended outcome, @dnzxy? Is rounding up part of the solution, or is the interval being extended so that rounding up does not need to take place? Or maybe both in some case?

@MikePlante1 can you share the test scenario you ran (FPU settings and example meal entry) you used to trigger rounding up?

dnzxy commented 1 month ago

Is this the intended outcome, @dnzxy? Is rounding up part of the solution, or is the interval being extended so that rounding up does not need to take place? Or maybe both in some case?

It is by implementation, so semi-intentional. We can either drop the "last" entry that needs rounding or we keep it, and round up to get to 1 gram. This stems from re-assigning equivalents to be 1g and still making sure that it "fits" into the total of equivalents.

I ran this example when I tested this feature during implementation with these settings: image

So the 7th entry should be 0.8g but is rounded up to be 1.0g, so +0.2g. Worst case would be an additional 0.9g of carbs some time down the line being added to COB and the algorithm would make a new dosing decision then.

I figured that is okay. What do you think?

avouspierre commented 1 month ago

tested with simulator and checked the code. LGTM !

bjornoleh commented 1 month ago

Any entry that converts to <1g carbs is rounded up to 1g.

Is this the intended outcome, @dnzxy? Is rounding up part of the solution, or is the interval being extended so that rounding up does not need to take place? Or maybe both in some case?

@MikePlante1 can you share the test scenario you ran (FPU settings and example meal entry) you used to trigger rounding up?

The rounding only applies to the last entry and will only amount to a single sub 1g rounding amount (if I understand correctly). I’d say this is acceptable.

You can test with the minimum amount of 1 g fat or 1 g protein, it will produce a single 1 g carb equivalent.

In the old implementation, the minimum values were 2 g fat or 5 G protein, which would produce 9-10 0,1 g equivalents (which are ignored). Less than 2 g fat or 5 g protein would result in nothing, which is more or less confusing too. I’d say it’s more intuitive to at least see that something is registered, and single gram changes are not expected to have much impact anyway.

aug0211 commented 1 month ago

I agree, I wouldn't block merging for that rounding. It's better than what existed before!

The scenario that stands out the most is of course an entry of 0/1/0 or 0/0/1 (C/F/P), where we end up with 1g of "FPU carbs" being logged for Trio to consider in calculation.

As others said here, 1g extra is probably not a big deal, though I do wonder if it'd be even better to check around line 170 of CarbStorage.swift and apply rounding to the last entry, so that if it nets out to < 0.5g it is cancelled as 0g, and if it nets out to >= 0.5g, it rounds to 1g?

MikePlante1 commented 1 month ago

If someone enters 1g of fat or protein, and expected it to log 0g of carb equivalents and therefore do nothing, they just shouldn’t have entered any fat or protein in the first place. Or is there a usecase for this that I’m not thinking of?

aug0211 commented 1 month ago

Yes, of course - mathematically consider any numeral that results in a clean calculation into FPUs (based on the static formula and user defined settings), with 1g of fat or protein remaining as a leftover.

bjornoleh commented 1 month ago

I'll go ahead and merge this now with four approving reviews. Well done! 🚀