-
Notifications
You must be signed in to change notification settings - Fork 407
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
Refactor and optimize storeCarbs
function to limit carb equivalents of FPU conversion to 1.0 grams
#240
Conversation
* Extracted the FPU processing logic into a new helper function `processFPU`, which ensures that each carb equivalent is at least 1.0 grams by adjusting the interval if necessary. * Added logic in new helper function`processFPU` to adjust the interval when the calculated carb equivalent is less than 1.0 grams. * Adjusted spread over time by computing duration again so total carb equivalents remain consistent * Refactored the main `storeCarbs` function to be more concise and easier to understand. * Introduced detailed docstrings to explain the purpose and functionality of the `storeCarbs`, `processFPU`, and `calculateComputedDuration` functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Only had a single comment. Approved!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Tested in a simulator with different settings and different fat/protein entries and looks to work as expected with carb entries converted from fat/protein entries never being less than 1 and reducing the number of entries to accomplish that. Any entry that converts to <1g carbs is rounded up to 1g. That sounds reasonable to me.
I compared the new code to the old code and didn't notice any discrepancies.
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? |
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:
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? |
tested with simulator and checked the code. LGTM ! |
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. |
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? |
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? |
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. |
I'll go ahead and merge this now with four approving reviews. Well done! 🚀 |
Merge pull request nightscout#240 from dnzxy/fix-fpu-conversion
Refactor and optimize `storeCarbs` function to limit carb equivalents of FPU conversion to 1.0 grams
Contents
This pull request refactors the
storeCarbs
function to improve readability, maintainability, and performance. The new version introduces a helper functionprocessFPU
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
processFPU
, which ensures that each carb equivalent is at least 1.0 grams by adjusting the interval if necessary.processFPU
to adjust the interval when the calculated carb equivalent is less than 1.0 grams.storeCarbs
function to be more concise and easier to understand.storeCarbs
,processFPU
, andcalculateComputedDuration
functions.Detailed Changes
storeCarbs
):processQueue.sync
.processFPU
.processFPU
):calculateComputedDuration
):