nightscout / cgm-remote-monitor

nightscout web monitor
GNU Affero General Public License v3.0
2.43k stars 71.82k forks source link

The DayToDay Report only uses the last basal profile, not the actual profiles #4991

Closed fromorbonia closed 4 years ago

fromorbonia commented 5 years ago

Describe the bug Where multiple basal profiles that apply over the period I want to report on - only one basal profile is applied, and it is always the latest one

To Reproduce Steps to reproduce the behaviour (in this example I am looking at one basal profile per day, starting on a Monday):

Expected behaviour Total basal units for Monday should be 6.0 units, Tuesday & Wednesday should be 11 units (unless temporary basals are in the dataset)

Setup information

Proposed Solution Root cause appears to be that profile.getTempBasal() function does not pull back correct basal profile based on date & time To fix that I have (so far) changed the following: In lib\profilefunction.js:

In lib\report-plugins\daytoday.js, I make changes to drawChart in the Basals data section to call profile.loadData to load the basal profiles, and the subsequent calls to profile.getTempBasal() return the correct basal profile.

fromorbonia commented 5 years ago

@jjaone - finally got around to do some proper testing on this.

fromorbonia commented 4 years ago

@jjaone - my changes are now in final testing, you can see them on the "wip/profiles" of my fork - https://github.com/fromorbonia/cgm-remote-monitor/tree/wip/profiles. I will be creating a PR soon so it will be in the main dev code

sulkaharo commented 4 years ago

@fromorbonia looks great! The system needs to additionally load the profile switch treatments, which can also contain a full profile. This is mostly used by AndroidAPS users as the main way to switch between profiles. Also ping @PetrOndrusek who's been looking into the profile system

fromorbonia commented 4 years ago

FYI, a detailed list of changes

In lib\profilefunctions.js:

In static/report/js/report.js:

In lib\report-plugins\daytoday.js:

In lib/report_plugins/profiles.js:

In lib/server/profile.js and lib/api/profile/index.js:

In profile.test.js:

sulkaharo commented 4 years ago

@fromorbonia Yup the changes look good! The missing part here is, the treatments collection can additionally contain Profile Switch treatments that contain profile data, which also needs to be loaded. The discussion with Petr has been to simplify this, so there'd be a single API call to load all the profile changes from a single API, so the code would not need to worry about mixed treatment and profile collection data. The reason I'm mentioning this is simply that if we merge this, the reports will still not render correct basals, until the data in treatments is handled.

PetrOndrusek commented 4 years ago

I think the transition to the new PS storage method will need more adjustments in the different parts of the system that we still need to discuss. Therefore, I would prefer not to associate it with this PR and left up as the next stage of adjustments.

fromorbonia commented 4 years ago

@sulkaharo - thanks for already looking at the code. I remember you mentioned about the profiles & treatments previously, but I couldn't spot what that meant in practice. In my particular setup, when I load a profile switch into the treatments there is no actual basal data, it relies on the profile data. Is there going to be a lot of people with that type of data mix? @PetrOndrusek - thanks for the confirmation about scope, it's taken me ages to get his far, so it will be really good to get this out onto the main branch! Happy to feed into any changes in PS Storage in the future.

jjaone commented 4 years ago

Thanks for working on this šŸ‘

hauraki commented 4 years ago

I'd also like to see a fix for this soon - is there any way I can help with this?

fromorbonia commented 4 years ago

@hauraki - do you have a test environment setup? If you want to use the code in my WIP branch above and test it with the profiles you loaded that would be really good. I'm just getting back to completing my own testing on this.

hauraki commented 4 years ago

@fromorbonia I tested your wip/profiles branch in my current setup (NS 13.0.2, Minimed 640G pump, 600SeriesAndroidUploader) and can confirm that changes to a given profile are displayed correctly in the d2d report.

Bildschirmfoto 2020-03-20 um 14 00 41 Here I just changed the basal rate between 1am and 2am for testing.

Bildschirmfoto 2020-03-20 um 14 03 40 Here I generated the report around the time of a bigger change in basal rate values.

FWIW, this all seems to work fine šŸ‘

fromorbonia commented 4 years ago

@hauraki - that is really good news, thanks for confirming that. I've put a PR in for the code, so should be in the next release.