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

Alarm / Threshold updates (and some other UI updates...) #305

Merged
merged 22 commits into from
Jan 6, 2015

Conversation

jasoncalabrese
Copy link
Member

Many users don't understand the current Loss/PredictAR based alarms, they also want to be able to set their own target range instead of the default 80-180.

This PR aims to make the target range and alarm types configurable, this should address most of #271. To do this 5 new ENV variables have been added.

  • BG_HIGH - default: 260, the high BG outside the target range that is considered urgent
  • BG_TARGET_TOP - default: 180, the top of the target range, also used to draw the line on the chart
  • BG_TARGET_BOTTOM - default: 80, the bottom of the target range, also used to draw the line on the chart
  • BG_LOW - default: 55, the low BG outside the target range that is considered urgent
  • ALARM_TYPES - default to simple if any BG_* ENV's are set, otherwise default to predict. There currently 2 alarm types are supported, and can be used independently are separately. The simple alarm type only compares the current BG to BG_ thresholds above, the predict alarm type works as before by using a highly tuned formula that forecasts where the BG is going based on it's trend.

@jasoncalabrese jasoncalabrese changed the title Alarm / Threshold updates Alarm / Threshold updates, for #271 Dec 19, 2014
@jasoncalabrese jasoncalabrese changed the title Alarm / Threshold updates, for #271 Alarm / Threshold updates Dec 19, 2014
@jasoncalabrese jasoncalabrese self-assigned this Dec 19, 2014
@scottleibrand
Copy link
Member

Do we really want to change the default behavior, and require an ENV variable to restore the previous default? Seems better to only change the alarm type if custom thresholds are defined

Scott

On Dec 19, 2014, at 12:45 PM, Jason Calabrese notifications@github.com wrote:

Maybe users don't understand the current Loss/PredictAR based alarms, they also want to be able to set their own target range instead of the default 80-180.

This PR aims to make the target range and alarm types configurable. To do this 5 new ENV variables have been added.

BG_HIGH - default: 240, the high BG outside the target range that is considered urgent
BG_TARGET_TOP - default: 180, the top of the target range, also used to draw the line on the chart
BG_TARGET_BOTTOM - default: 70, the bottom of the target range, also used to draw the line on the chart
BG_LOW - default: 55, the low BG outside the target range that is considered urgent
ALARM_TYPES - default: simple, currently 2 alarm types are supported, and can be used independently are separately. The simple alarm type only compares the current BG to BG_ thresholds above, the predict alarm type works as before by using a highly tuned formula that forecasts where the BG is going based on it's trend.
You can merge this Pull Request by running

git pull https://github.com/nightscout/cgm-remote-monitor wip/jingle-bells
Or view, comment on, or merge it at:

#305

Commit Summary

read BG thresholds and alarm types from ENV and make them available to the server and api
read BG thresholds from api; refactor init so that it happend after we get basic settings from api
emit alarms based on ENV ALARM_TYPES (simple and predict)
updated BG_HIGH default to 240
File Changes

M env.js (9)
M lib/api/index.js (2)
M lib/api/status.js (1)
M lib/websocket.js (45)
M static/js/client.js (519)
M static/js/ui-utils.js (22)
Patch Links:

https://github.com/nightscout/cgm-remote-monitor/pull/305.patch
https://github.com/nightscout/cgm-remote-monitor/pull/305.diff

Reply to this email directly or view it on GitHub.

@jasoncalabrese
Copy link
Member Author

@scottleibrand I've been trying to decide on that. I think most users expect an alarm below the line on the chart. The easy approach would be to default ALARM_TYPES to "predict" or even "simple predict".

@jasoncalabrese
Copy link
Member Author

Also thinking about drawing a line at BG_HIGH and BG_LOW, then green in target, yellow between target and BG_HIGH/BG_LOW, and red above and below. With that urgent is red and normal is yellow.

@scottleibrand
Copy link
Member

IMO, if a user sets a threshold, it should simply alarm at that threshold unless they specify to predict. If the user does nothing, the current behavior should not change: some people rely on Nightscout "alarming sooner" than Dexcom.

Scott

On Dec 19, 2014, at 1:03 PM, Jason Calabrese notifications@github.com wrote:

@scottleibrand I've been trying to decide on that. I think most users expect an alarm below the line on the chart. The easy approach would be to default ALARM_TYPES to "predict" or even "simple predict".


Reply to this email directly or view it on GitHub.

@jasoncalabrese
Copy link
Member Author

I'll go with that for now

Jason Calabrese added 2 commits December 19, 2014 13:17
@raven569
Copy link

Where do we need to setup the ENV to use this? On the code env.js or in
azure?

Maybe users don't understand the current Loss/PredictAR based alarms, they
also want to be able to set their own target range instead of the default
80-180.

This PR aims to make the target range and alarm types configurable. To do
this 5 new ENV variables have been added.

  • BG_HIGH - default: 240, the high BG outside the target range that
    is considered urgent
  • BG_TARGET_TOP - default: 180, the top of the target range, also
    used to draw the line on the chart
  • BG_TARGET_BOTTOM - default: 70, the bottom of the target range,
    also used to draw the line on the chart
  • BG_LOW - default: 55, the low BG outside the target range that is
    considered urgent
  • ALARM_TYPES - default: simple, currently 2 alarm types are
    supported, and can be used independently are separately. The simple
    alarm type only compares the current BG to BG_ thresholds above, the
    predict alarm type works as before by using a highly tuned formula
    that forecasts where the BG is going based on it's trend.

You can merge this Pull Request by running

git pull https://github.com/nightscout/cgm-remote-monitor wip/jingle-bells

Or view, comment on, or merge it at:

#305
Commit Summary

  • read BG thresholds and alarm types from ENV and make them available
    to the server and api
  • read BG thresholds from api; refactor init so that it happend after
    we get basic settings from api
  • emit alarms based on ENV ALARM_TYPES (simple and predict)
  • updated BG_HIGH default to 240

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#305.

@jasoncalabrese
Copy link
Member Author

@raven569 you can set them in the Azure settings, just like the ENV vars used here: http://www.nightscout.info/wiki/labs/cgm-remote-monitor-care-portal

@raven569
Copy link

So have to add all 5 variables?

@raven569 https://github.com/raven569 you can set them in the Azure
settings, just like the ENV vars used here:
http://www.nightscout.info/wiki/labs/cgm-remote-monitor-care-portal


Reply to this email directly or view it on GitHub
#305 (comment)
.

@raven569
Copy link

Sot just add ALARM_TYPES and predict in azure yes?

@raven569 https://github.com/raven569 you can set them in the Azure
settings, just like the ENV vars used here:
http://www.nightscout.info/wiki/labs/cgm-remote-monitor-care-portal


Reply to this email directly or view it on GitHub
#305 (comment)
.

@jasoncalabrese
Copy link
Member Author

All the new settings have a default, so you only need to change the ones you want.

For example if you like the default BG_* thresholds, but if you wanted simple alarms you could just set ALARM_TYPES="simple"

@raven569
Copy link

I want the 240 and turn off the 180. Code has bottom at 80 not 70? Want the
55 on and 80 off.

All the new setting have a default, so you only need to change the ones
you want.

For example if you like the default BG_* thresholds, but if you wanted
simple alarms you could just set ALARM_TYPES="simple"


Reply to this email directly or view it on GitHub
#305 (comment)
.

@jasoncalabrese
Copy link
Member Author

It's probably best to set all the env vars, then you don't have to remember what the defaults are.

Notes above were wrong 70 vs 80, thanks for catching it, fixing

@raven569
Copy link

OK so how set all? Just put predict ? Or do u call each one?

It's probably best to set all the env vars, then you don't have to
remember what the defaults are.

Notes above were wrong 70 vs 80, thanks for catching it, fixing


Reply to this email directly or view it on GitHub
#305 (comment)
.

var lastBG = actual[actualLength].y;

if (lastBG > env.thresholds.bg_high) {
emitAlarmType = 'urgent_alarm';
Copy link
Member

Choose a reason for hiding this comment

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

Let's not conflate urgent and high.
This should be a high alarm type not urgent, right?

Copy link
Member

Choose a reason for hiding this comment

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

Jason stipulated two high thresholds: a "high end of target range"
threshold (default 180), which would trigger a standard alarm, and a
"urgent high" threshold (default 240) that would trigger an urgent alarm.
Similar on the low end (80 and 55 IIRC).

Does that paradigm make sense to you? Do you have any suggestions for a
better way to approach it?

On Fri, Dec 19, 2014 at 3:56 PM, Ben West notifications@github.com wrote:

In lib/websocket.js
#305 (diff)
:

@@ -238,20 +238,42 @@ function loadData() {
emitData( );
}

  •    // compute current loss
    
  •    var avgLoss = 0;
    
  •    var size = Math.min(predicted.length - 1, 6);
    
  •    for (var j = 0; j <= size; j++) {
    
  •        avgLoss += 1 / size \* Math.pow(log10(predicted[j].y / 120), 2);
    
  •    var emitAlarmType = null;
    
  •    if (env.alarm_types.indexOf("simple") > -1) {
    
  •        var lastBG = actual[actualLength].y;
    
  •        if (lastBG > env.thresholds.bg_high) {
    
  •            emitAlarmType = 'urgent_alarm';
    

Let's not conflate urgent and high.
This should be a high alarm type not urgent, right?


Reply to this email directly or view it on GitHub
https://github.com/nightscout/cgm-remote-monitor/pull/305/files#r22136552
.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was attempting to not change the current normal/urgent alarms levels, since adding extra levels will make snooze more complicated.

There are still issues to work out with the High/Low setting on the client. The easy thing to do is make them say Urgent/Normal, but I don't think thats what people want.

@jasoncalabrese
Copy link
Member Author

Lots of updates

Added settings to enable/disable the different alarms types:
screen shot 2014-12-19 at 11 28 14 pm

Normal alarms are now yellow, urgent alarms are red:
screen shot 2014-12-20 at 12 12 07 am

Now showing 4 lines (high/low are lighter), red is now for urgent high/low, with yellow in-between red and green:
screen shot 2014-12-19 at 11 27 48 pm

@jasoncalabrese
Copy link
Member Author

@scottleibrand I'm wondering if the default for ALARM_TYPES should be "simple predict", then you'd get the hybrid mode. I'll see what everyone else thinks after testing this.

@ELUTE
Copy link
Contributor

ELUTE commented Dec 22, 2014

Agreed. Would love to see that!

Sent from my iPad

On Dec 22, 2014, at 2:35 PM, Jim Sifferle notifications@github.com wrote:

@jasoncalabrese So far so good on Jingle Bells alarms.

Should CP entries w/o carbs or insulin still show up in their previous position as a grey dot above the trend line? Would you consider putting BG checks on the trend line, positioned at the tested BG #? It could be colored green/yellow/red depending on where it sits in the alarm thresholds... or just a static color. What about other types of CP entries?


Reply to this email directly or view it on GitHub.

@jasoncalabrese
Copy link
Member Author

I think we need a BG dot for 3 types: calibration, meter(ShugaTrak), and BG check(manual, care portal)

Thinking same red dot with different border

@raven569
Copy link

BG checks would be interesting... If they were shown on position of BG
check value at time... See diff in Dex vs BG values.

Agreed. Would love to see that!

Sent from my iPad

On Dec 22, 2014, at 2:35 PM, Jim Sifferle notifications@github.com
wrote:

@jasoncalabrese So far so good on Jingle Bells alarms.

Should CP entries w/o carbs or insulin still show up in their previous
position as a grey dot above the trend line? Would you consider putting BG
checks on the trend line, positioned at the tested BG #? It could be
colored green/yellow/red depending on where it sits in the alarm
thresholds... or just a static color. What about other types of CP entries?


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub
#305 (comment)
.

@jasoncalabrese
Copy link
Member Author

I have the code to estimate BG position by time, so we could put everything on the trend line, but I think it could get crowded. Some events like basal and activity make sense at the top.

@jimsiff
Copy link
Contributor

jimsiff commented Dec 22, 2014

@jasoncalabrese Should there be different sized dots for different CP entry types? I see a large dot for a standard meal bolus, medium dots for a split entry (prebolus) and smaller dots for an insulin correction.

Edit: Or is the CP dot size related to the amount of carbs / insulin given?

screen shot 2014-12-22 at 2 50 16 pm

screen shot 2014-12-22 at 2 55 41 pm

screen shot 2014-12-22 at 2 55 24 pm

@jasoncalabrese
Copy link
Member Author

Yes, the size is relative, it would work even better if it knew the actual carb ratio that was used.

@jasoncalabrese
Copy link
Member Author

@bewest you'll like 38f755e, found the fix on @ldesboro's site :)

screen shot 2014-12-23 at 5 12 11 pm

@rnpenguin
Copy link
Contributor

nice :)

Jason Calabrese added 2 commits December 26, 2014 23:27
@@ -238,6 +242,11 @@ div.tooltip {
font-size: 70%;
}

html body #bgButton {
Copy link
Contributor

Choose a reason for hiding this comment

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

html body is unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, should this be merged with the #bgButton rule on line 153?

@bewest
Copy link
Member

bewest commented Jan 3, 2015

Looking good.

@trhodeos
Copy link
Contributor

trhodeos commented Jan 4, 2015

Added a few comments. Looks good otherwise.

@jasoncalabrese
Copy link
Member Author

@trhodeos thanks for the code review, think I took care of those issues

I merged this to my prod branch and will be using it all day tomorrow, if there aren't any new issues I plan to merge tomorrow.

jasoncalabrese added a commit that referenced this pull request Jan 6, 2015
Alarm / Threshold updates (and some other UI updates...)
@jasoncalabrese jasoncalabrese merged commit 14d5568 into dev Jan 6, 2015
This was referenced Jan 6, 2015
@jasoncalabrese jasoncalabrese deleted the wip/jingle-bells branch January 6, 2015 07:05
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.

8 participants