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

Variability reporting, Webpack bundling #2736

Merged
merged 23 commits into from
Aug 23, 2017

Conversation

sulkaharo
Copy link
Member

@sulkaharo sulkaharo commented Aug 11, 2017

  • Glucose data smoothing / patching before calculating variability to reduce the effects of CGM noise
  • Added calculation for GVI, PGS and Total Absolute Change calculation
  • Refactored the server to use Webpack for the bundle generation instead of Browserify (since browserify-express seems to not work with Node 8 and is not maintained)
  • Improved caching & compression of JS and CSS resources
  • If NODE_ENV="development”, don’t cache resources for more than 10 seconds & generate a report about Bundle size

This is still WIP, issuing PR so others can test

TODO:

  • Remove the rest of Browserify artifacts
  • Later: runtime bundle regeneration
  • CGM data filtering looks to have a bug with removing noisy data, which needs to be checked

… reduce the effects of CGM noise

* Added calculation for GVI, PGS and Total Absolute Change calculation
* Refactored the server to use Webpack for the bundle generation instead of Browserify (since browserify-express seems to not work with Node 8 and is not maintained)
* Improved caching & compression of JS and CSS resources
* If NODE_ENV="development”, don’t cache resources for more than 10 seconds & generate a report about Bundle size
@stavlor
Copy link
Contributor

stavlor commented Aug 11, 2017

:+1

@jasoncalabrese
Copy link
Member

I wonder why multiple browserify packages are listed in the package-lock.json, do we need to use an npm command to remove them? Are maybe they're a dependency of something else?

@jasoncalabrese
Copy link
Member

I was expecting to see a new npm script to do the webpacking, but looks like you set it up to do run webpack as part of starting the server?

When I try to run it locally I'm getting this error:

WARNING: express-minify cache directory is not writeable, fallback to memory cache.
/Users/jason/Development/nightscout/crm-upgrade/node_modules/mongodb/lib/utils.js:123
    process.nextTick(function() { throw err; });
                                  ^

Error: Cannot find module 'punycode/'

maybe missing a dependency?

Copy link
Member

@jasoncalabrese jasoncalabrese left a comment

Choose a reason for hiding this comment

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

now working for me locally

@@ -25,12 +25,16 @@
<meta name="msapplication-config" content="/browserconfig.xml">
<meta name="theme-color" content="#333333">

<link rel="stylesheet" type="text/css" href="/css/drawer.css?v=0.10.0-dev-20170716" />

<!-- <link rel="stylesheet" type="text/css" href="/css/drawer.css?v=0.10.0-dev-20170716" />
Copy link
Member

Choose a reason for hiding this comment

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

remove commented out links

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch

@stavlor
Copy link
Contributor

stavlor commented Aug 15, 2017

Been working well for me been running it for about 3-4 hours now on GCE with no issues

@sulkaharo
Copy link
Member Author

Bower is now gone. Font needs adjusting; the previous look came from loading a very complex set of CSS from Google, which is a no-go for performance. Would love more testing. ;)

@sulkaharo
Copy link
Member Author

/radio.html was deleted as part of the repackaging and /reports/compare doesn't work, not sure if it's worth fixing.

@stavlor
Copy link
Contributor

stavlor commented Aug 16, 2017

Wow nice to see bower gone, that should be interesting will update my deployment to use this in a bit and let you know if I notice anything odd.

Copy link
Contributor

@stavlor stavlor left a comment

Choose a reason for hiding this comment

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

Looks good, been working well so far as well.

@PieterGit
Copy link
Contributor

PieterGit commented Aug 20, 2017

Got this branch running. Installs fine. I upgraded node on my branch to 8.4.0 and fix the UTC problem for treatments. See https://github.com/PieterGit/cgm-remote-monitor/tree/treatments_utc

This branch really speeds up Nightscout. Thanks for that!

Two issues I noticed:

  • With a mobile, I sometimes only see a black screen with time and careportal buttons. Refreshing fixes the problem. I think this also was the case before this PR.

  • PGS Seem to differ each day, ranging from 9.7 to 46.6. That seems too good for a PWD (according to the link up to 35 is for non diabetes). I will look into this later.

@PieterGit
Copy link
Contributor

Feature request: can the GVI and PGS also be displayed in the Daily Stats and/or the Weekly success?
It's now only displayed on the Distribution tab.

Is the Percentage Time in Range (PTIR) the same as the % of Readings for range Normal?

@jasoncalabrese
Copy link
Member

jasoncalabrese commented Aug 21, 2017

I'm seeing some css glitches, borders on pills aren't uniform, probably related to the font change. Is it depending on system fonts? Also probably related to the issue @PieterGit reported the toolbar buttons on a phone aren't overlaying the top of the bg like before and use a ton more space.

Update: Pill issue is that the battery icon isn't showing

@stavlor
Copy link
Contributor

stavlor commented Aug 21, 2017

Yeah looking at mine, I see the percentages but no icon.

@stavlor
Copy link
Contributor

stavlor commented Aug 21, 2017

Actually note normally I could click on my battery pill to get a tooltip with full battery status I currently can get no tooltip for battery percentage.

@sulkaharo
Copy link
Member Author

CSS might need some adjusting for pill uniformity. The non-optimized site loads 10 webfonts to show the pills, while this one loads just one. Tooltips should work, no idea why those would break. Which browsers are you using to view the site?

@sulkaharo
Copy link
Member Author

Battery icon fixed. Clicking on the battery doesn't do anything for me in any version of Nightscout, so it's hard for me to debug what's going on if it's supposed to be clickable. :(

Excellent suggestion on making the metrics more generally available, adding. @PieterGit yes my understanding is PTIR == time in normal range. I double checked the math (you can see more output by looking at the console in web dev tools) and I think the issue with calculating PGS is the metric is extremely dependent on what the target range is and if you have good control (for a diabetic) and are using a wider range for the target, the (1-PTIR) multiplier goes extremely low. I'm usually looking at us being in range of 3.7 to 10 mmol, which gives us >90% time in range, which drops the PGS to non-diabetic range. The linked article fails to mention what targets this has been designed against; might be worth hard-coding this to a specific range.

@sulkaharo
Copy link
Member Author

Some of the tests are fixed now; had to change some of the benv setups to run correctly with the new dependencies. One test is failing in what looks like a teardown issue (it works if invoked alone). The profile tests are breaking in code that should not have changed, don't know what's going on there. Report test seems to have some issue with multiple client instances being running at the same time, with conflicting sandboxing(?).

@PieterGit
Copy link
Contributor

Dexcom Studio even uses 4,44 to 7,21 mmol as default range. I upgraded my Dexcom studio from
12.0.4.6 to 12.0.5.15. I hoped the newest version has the GVI and PGS indicators, but I can't find GVI or PGS in the interface unfortunately. It would be good to have the same data cleansing and target range as Dexcom suggest.

@sulkaharo
Copy link
Member Author

Merging this; taking too long to PR so risking large conflicts. If anyone finds issues with caching, please let me know.

@sulkaharo sulkaharo merged commit 2afafab into nightscout:dev Aug 23, 2017
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.

4 participants