Skip to content
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

Add ability to calculate glucose noise #1298

Merged
merged 11 commits into from
Nov 10, 2019

Conversation

jpcunningh
Copy link
Contributor

This adds the option for oref0 to calculate the glucose noise level prior based on the glucose history.

It is disabled by default and controlled with the calc_glucose_noise preference.

Copy link
Contributor

@scottleibrand scottleibrand left a comment

Choose a reason for hiding this comment

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

Couple comments inline. Overall questions:
Where did this algorithm come from? Is it in use (has it been tested) elsewhere?
What happens if the CGM provides a noise value and the calculate glucose noise preference is true?

@@ -917,6 +920,14 @@ function compare_with_fullhistory() {
fi
}

function update_glucose_noise() {
if check_pref_bool .calc_glucose_noise false; then
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems backwards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to work right. It defaults to false (the second argument to check_pref_bool) if .calc_glucose_noise is not set, so the if expression only evaluates to true if .calc_glucose_noise is in the preferences.json file and is set to true.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, that's a really confusing implementation of check_pref_bool (not your fault). I read it to be equivalent to "if calc_glucose_noise === false".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! that's the way I wrote it first, but it didn't work! 😄

lib/glucose-stats.js Outdated Show resolved Hide resolved
@jpcunningh
Copy link
Contributor Author

@efidoman originally implemented it in Logger and I ported it to Javascript for Lookout. I believe Logger has a different algorithm in use now.

It's currently in use in Lookout. We have had good results for well over a year. It's response for us has mapped very well to the ??? reported by the official Dexcom algorithm when a sensor gets noisy.

It overwrites any CGM provided noise value if calc_glucose_noise is true.

@scottleibrand
Copy link
Contributor

Do you have any examples of a scenario where the CGM provides noise and we'd want to overwrite it?

@jpcunningh
Copy link
Contributor Author

For discussion on the original algorithm implemented in Logger: https://gitter.im/thebookins/xdrip-js?at=5a88098a35dd17022eb64c4e

@jpcunningh
Copy link
Contributor Author

I think ultimately we would want to override the CGM provided noise value if we calculate a medium or heavy noise, but the CGM is sending a clean noise value.

Or, in the case of the current state of the code, if one is using a CGM source they know does not correctly report noise, they can enable the option in oref0 to calculate the noise value and overwrite the CGM reported noise.

@scottleibrand
Copy link
Contributor

Is it possible for the CGM to report medium or heavy noise, but for us to calculate light or zero noise and override the CGM? Seems like we'd want to take the max() of the two.

@jpcunningh
Copy link
Contributor Author

Added logic to do as you suggested - use the highest noise value between the calculated noise and the CGM reported noise.

Copy link
Contributor

@scottleibrand scottleibrand left a comment

Choose a reason for hiding this comment

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

Cool, thanks. LGTM.

@scottleibrand
Copy link
Contributor

Anyone want to volunteer to test this?

@TwistaTim
Copy link
Contributor

@scottleibrand I can test this.....

@scottleibrand
Copy link
Contributor

@Tornado-Tim any progress on testing?

@scottleibrand
Copy link
Contributor

Should we go ahead and merge this into dev for testing there?

@jpcunningh
Copy link
Contributor Author

We've been running it for a month without any noticeable issues... I believe it can be safely merge it to dev.

@danamlewis danamlewis merged commit ee1cdf3 into openaps:dev Nov 10, 2019
@jpcunningh jpcunningh deleted the glucose-noise-measurement branch November 11, 2019 03:43

const x2x1Delta = xarr[i] - xarr[i - 1];

if ((lastDelta > 0) && (y2y1Delta < 0)) {
Copy link

Choose a reason for hiding this comment

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

@jpcunningh lastDelta is a never-updated const set to 0 in line 49. Given the name I'd expect this to be set at the end of the loop. Like it is now, this if and following else if never match, do they?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @jotomo! You are right, the assignment line was missing.

@danamlewis danamlewis mentioned this pull request Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants