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 autotune report to Nightscout #3191

Closed
wants to merge 99 commits into from
Closed

Add autotune report to Nightscout #3191

wants to merge 99 commits into from

Conversation

scottleibrand
Copy link
Member

This adds autotune as an option to NS reports.

Huge thanks to @lukas-ondriga, @martinpetlus, @vstrisovsky, @mdr, @marek0o, who did most of the heavy lifting on this months ago.  And a big thanks to @jessiepusateri for making me re-aware of their work.

To complete the integration, I refactored the autotune libraries, so that they are identical between OpenAPS and NS, and we can simply cp over each file from oref0 to cgm-remote-monitor and commit it in both places whenever we make updates.

With the speed optimizations I just made to autotune, the autotune NS report now runs on up to 2 weeks of data in under a minute, and can analyze 30 days' worth in about 3 minutes. I would recommend starting with 3 days' or 1 week of data. If you run it on 2 weeks or more, Chrome will give you a warning dialog after 30 seconds indicating that the tab is non-responsive: just Wait and allow it to finish.

@danamlewis also added some introductory language and caveats.  If anyone has any other ideas for making things clearer, suggestions (or better yet, PRs to the autotune branch) are welcome. Once this nears merging, @danamlewis also plans to update the autotune docs to reflect the ease of use.

(Note: this version of autotune still works solely for single values ISF and carb ratio. It’ll tune the first or last value in the profile, even if there are multiples. Some language or warnings for multiple-profile runs may be needed to be added, and volunteers welcome for working with us to figure out a multiple-schedule version of autotune in the future.)

screenshot 2017-12-24 21 26 04

screenshot 2017-12-24 21 26 19

dbeasy and others added 7 commits April 23, 2018 22:15
Many email GUI's place newest on top, less scrolling, adjusting my nightscout to do that too, since I have to keep clicking it anyways! -dboland
* npm update and test if nodejs 9.x (currently 9.7.1) also works

* explicit node version in .travis.yml

* add webpack-cli

* UglifyJsPlugin is deprecated in webpack 4. It's used as --optimize-minimize as default

* improve Readme

* update some more

* fix typo in .travis.yml and remove node 9.7.1 because Travis does not seem to support it

* upgrade simple-statistics.

Use standardDeviation instead of standard_deviation. Other functions are not changed.
try node 9 on travis

* downgrade async to 1.5.2 to see if that makes the test work again

* downgrade async module and fix tests

* npm update

* update npm in package.json

* restore RMS in glucosedistribution

* remove unused total
use SI units (L instead of l, min instead of m)

* improve mongo

* apply fix suggested by Paul Andrel @stavlor, see https://gitter.im/nightscout/public?at=5ab542f135dd17022e92891a

* npm update

* fix mongodb, remove ancient npm versions. azure has 5.7
j

* revert to simple-stats 0.7.0

* add bundle-dev option to package.json

* fix distribution test

* upgrade nvm version for travis to latest nvm.

once node 8 and node 9 ship with npm > 5.6.0 we should remove this. see npm/npm#17858 for details
@nightscout nightscout deleted a comment Apr 24, 2018
@nightscout nightscout deleted a comment Apr 24, 2018
@nightscout nightscout deleted a comment May 18, 2018
@PieterGit
Copy link
Contributor

@scottleibrand can you give an update on this PR. Is it ready to merge to dev (after resolving the conflict), or should we postpone it to a next Nightscout release?

@scottleibrand
Copy link
Member Author

The most recent update (from @triplem) is that "the difference between Firefox and Chrome seems to be, that Firefox is losing connection via websockets sometimes which seems to be not happening on Chrome". The error is "The connection to ws://localhost:1337/socket.io/?EIO=3&transport=websocket&sid=vy0Wg1-Ir1U_aKXHAAAh was interrupted while the page was loading".

However, it's unclear whether that's actually causing differences in final results. @sulkaharo said he was going to try to find time to compare the results from the rig vs. NS, but I haven't heard anything further on that.

@philipgo
Copy link
Contributor

philipgo commented Jul 6, 2018

After these two changes ( #3562 and #3563 ) I get preparation results that are identical between Chrome and Firefox and almost identical to OpenAPS dev. Before, this step failed on Firefox due to #3562 and results differed between OpenAPS and Chrome due to #3563 .

However, even though data preparation appears to be correct, I still get different end results on all three (OpenAPS, Chrome, Firefox)

@PieterGit
Copy link
Contributor

@sulkaharo @scottleibrand, @danamlewis, @lukas-ondriga, @martinpetlus, @vstrisovsky, @mdr, @marek0o, @jessiepusateri, @philipgo I think this autotune PR is almost ready to merge to dev for the 0.11 release. Can somebody resolve the merge conflict and also confirm this PR is in good state?

@philipgo reports that the results of the autotune run are identical between Chrome and Firefox and almost identical to OpenAPS dev. Can others also check this, and let's try to explain, fix or accept these (small) differences.

@sulkaharo
Copy link
Member

Given the results still vary and nobody has figured out the individual hours having discrepancies in the results, I wouldn't merge this until the results are consistent across the board. I'll check how close the results from here are compared to what openaps tells us asap but can as many people as possible take a look at their results and report back how close are the reports here vs autotune on an openaps rig?

@PieterGit PieterGit mentioned this pull request Aug 25, 2018
@diggabyte
Copy link

Any chance we can itemize the outstanding issues that need to be resolved here? Happy to contribute.

@PieterGit
Copy link
Contributor

@diggabyte I still think this PR need testing as suggested by @sulkaharo on August 6th.
@scottleibrand Do you think it feasible to get this PR in a proper state on short term? What needs to be done before merging?

@PieterGit
Copy link
Contributor

@scottleibrand I know there is interest in this PR, but I don't see any activity for the last months. If there is no activity in the next month I will close this PR unfinished. If we want to test and fix it, we can restart it, but leaving half implemented unfinished PR's open for almost a year is not the way to go.

@scottleibrand
Copy link
Member Author

Ok, I think we should close this for now, and re-open if we decide to move forward. At the moment, autotuneweb looks like a more promising approach, particularly once timezone support is added... If @danamlewis or anyone else concurs, feel free to close the issue. Probably best not to delete the autotune branch just yet, though.

@PieterGit PieterGit removed this from the 0.11.0 milestone Oct 25, 2018
@PieterGit
Copy link
Contributor

PieterGit commented Oct 25, 2018

@scottleibrand @danamlewis Closing PR due to inactivity. Current PR still has issues and must be tested properly. Postponing for now. Keeping the autotune branch available if people want to work in it later. If somebody is interested in finishing it: please resolve merge errors, resolve codacy errors and make sure that it gives consistent output with several browsers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet