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

bg data smoothing #2141

Closed
wants to merge 3 commits into from
Closed

bg data smoothing #2141

wants to merge 3 commits into from

Conversation

justmara
Copy link

@justmara justmara commented Oct 25, 2022

This is incoming BG data smoothing taken from Tsunami project (I've contacted author long time ago, he has nothing against this move, but for some reason still didn't managed to make a PR).

It is a switchable option under 'Overview' settings group.
image

It uses sliding weighted average algo for calculating smoother curve. This acts much better than existing use long_delta.

This can be used as a replace of 'advanced filtering' option to allow SMB usage on any data source.

@justmara
Copy link
Author

example
IMG_20221025_172256_402
IMG_20221025_172250_818

@swissalpine
Copy link
Contributor

swissalpine commented Oct 29, 2022

The plugin works as @justmara described. The fact that not only back smoothing takes place, but that the current value is also modified (and deviates from the display in xDrip+) is unusual at first. But at second view it is plausible.
Very interesting!

@MilosKozak
Copy link
Contributor

well smoothing actual value leads to slowing down reaction to bg changes which not happen when backsmoothing
I like the idea of smoothing bg but i'd prefer backsmoothing
jumpy values are only one of the reason of disabling SMB always. we could enable it for some bg sources but not for Libre 2 ( for example) were the issue is producing wrong values instead of error state

@osodebailar
Copy link
Contributor

@MilosKozak one of the future problems
Can be using G7 instead of G6 with upcoming BOYDA. The new G7 app has no more backsmoothing in the moment .

@justmara
Copy link
Author

justmara commented Nov 2, 2022

It adds minor slowdown of AAPS reaction for instant big changes. But that IS the point of smoothing itself - to leverage impact of occasional changes and give AAPS strong reliable data to work with.

@khskekec
Copy link

khskekec commented Nov 3, 2022

@osodebailar Actually we are testing the first prototype with @blaqone together and the general broadcasting from G7 app to AAPS is working. Also backfilling capabilities are possible.

We are just struggling with the data handling in AAPS but this is not a big deal at first look.

@justmara It looks wonderful and is a great approach to also support stupid G7 sensors in the future. This will make the implementation in the G7 app obsolete.

As i see there are some conflicts here. Do you plan to resolve them?

@justmara
Copy link
Author

justmara commented Nov 3, 2022

@khskekec rebased on latest dev

@khskekec
Copy link

khskekec commented Nov 4, 2022

Hey @justmara ,

maybe i can help you just more with some additional data broadcasted from G7 to make the algorithm more solid?

The following images are showing the available data for G7 sensor readings. I am not sure what data could be helpful for you but who knows...

Bildschirmfoto 2022-11-04 um 18 45 13

Bildschirmfoto 2022-11-04 um 18 45 28

Bildschirmfoto 2022-11-04 um 18 46 47

Just let me know :)

@justmara
Copy link
Author

justmara commented Nov 4, 2022

@khskekec this code is sensor-agnostic. It works regardless of bg data source and does not have any of the particular source's additional data. It works over the AAPS GlucoseValue abstraction. So it won't have any special branch for G7 or any other sensor because it does not rely on sensor type at all.
And Iam not the original author for it. As I've stated - I took from Tsunami AAPS project. You can reach its author and ask him your questions if you want to do any additional tune :)

@blaqone
Copy link

blaqone commented Nov 5, 2022

Love it! Please add!

@Der-Schubi
Copy link

Der-Schubi commented Nov 6, 2022

This really helps, when sensors get crappy! Using it for 10 days now without any problems from the slowed down reaction time.

Crappy 13 days old G6 in xDrip:
smoothing_xdrip
And smoothed data in AAPS:
smoothing_aaps

@justmara
Copy link
Author

justmara commented Nov 9, 2022

another example :)

Screenshot_20221109-094340_xDrip+
Screenshot_20221109-094259_NSClient

@justmara
Copy link
Author

justmara commented Nov 9, 2022

and rebased on dev once again

@szantos
Copy link

szantos commented Nov 10, 2022

Basically very good approach!
Is it possible to blend original and smoothed data into one graph to be able to see better how the values are affected?
Original vs smoothed deltas would be good, too.

@justmara
Copy link
Author

justmara commented Nov 10, 2022

@szantos something like this?

image

and a light theme

image

and with smoothing switched off

image

for my opinion it makes graph even more overloaded. there is too much info on it. maybe i can be added with additional selection option from dropdown - didnt look how that could be made

@justmara
Copy link
Author

holy crap, three rebases in a row and every time like whole project is mixed and moved around. some files are even deleted so i give up fixing this stuff up for now. will keep using it for myself then.

@Der-Schubi
Copy link

holy crap, three rebases in a row and every time like whole project is mixed and moved around. some files are even deleted so i give up fixing this stuff up for now. will keep using it for myself then.

All PRs have these problems at the moment, there is quiet some movement in the files right now. Just wait a week or two. I Really would love to have this merged!

@khskekec
Copy link

That's a nice one 👍

@szantos
Copy link

szantos commented Nov 11, 2022

@szantos something like this?

@justmara
Yes! Best way is directly in AAPS, like you did. Being switchable is a good idea. Raw spots should be drawen on top of the smoothed ones.

What I can see from it:

  • it adds around 5 min. of lag (for SGV and delta)
  • deltas are mainly similar
  • smoothing itself seems to be right in some cases, but not always

The effect on local extreme values is not clear to me. At 19:25 smoothed value is lower, at 20:15 higher than raw. Which one is right is hard to tell.

19:30-20:00 seems to be a fake low for me, but this smoothing is not intended to detect or correct it.

The main reason I've asked for a 1:1 comparison ability is to make users able to better understand and judge the effect of this smoothing algorithm.

Personally I'm happy about all kind of improvements which extend our toolbox, since they can be toggled on/off and not forced. Hope you get it merged!

Danke!

@justmara
Copy link
Author

@szantos

19:30-20:00 seems to be a fake low for me, but this smoothing is not intended to detect or correct it.

nope, it was real low handled with juice. but there was fake high at 19:25 (6mmol) - sometimes libre gives such one-point jumps and this smoother handles'em nicely.

@mxsrm
Copy link

mxsrm commented Nov 11, 2022

This is a brilliant addon, thank you for it!

reverted bolus icon accidentally crawled in to previous commit:)

(cherry picked from commit 47b5472)
@justmara
Copy link
Author

Added menu selector for raw data on main graph.

image

image

@osodebailar
Copy link
Contributor

Very cool.
Smoothing works perfect for me with G7

@blaqone
Copy link

blaqone commented Nov 13, 2022

Yea works perfectly here too. Added it to the main branch, added autoIFS and g7 Support. So far: flawless!

@sonarcloud
Copy link

sonarcloud bot commented Nov 13, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug D 29 Bugs
Vulnerability D 3 Vulnerabilities
Security Hotspot E 41 Security Hotspots
Code Smell A 1636 Code Smells

No Coverage information No Coverage information
2.2% 2.2% Duplication

@khskekec
Copy link

Hey,

I just prepared nightscout to get and show smoothed data points across the raw values. As I know the smoothing just happens in Cgm Transaction but I think this is A very bad place to put interaction with nightscout. What is in your opinion a preferable place?

And... is it correct that only the latest 10 readings will become smoothed?

@tim2000s
Copy link
Contributor

While I understand everyone's desire for super smoothed BG data, I don't know whether most people are aware that SMB existed well before the G6 and back smoothing did.

It was never a requirement for using SMB. The original reason for preferring Dexcom was that the Dexcom data had a noise indicator in it (it still does) so OpenAPS could moderate it's behaviour based on that noise indication.

@Der-Schubi
Copy link

Der-Schubi commented Nov 22, 2022

holy crap, three rebases in a row and every time like whole project is mixed and moved around. some files are even deleted so i give up fixing this stuff up for now. will keep using it for myself then.

Hi! Could you please update / rebase the PR? Milos is save of nitpicks done with moving things around.

@piecycle
Copy link

piecycle commented Nov 26, 2022

I'm the author of this code and have just been made aware about this PR from Mara's own repository. Sorry for being late to the party, I wasn't informed about this discussion, but as Mara said I am happy to make the code available to the community, especially as the feedback has been so positive. :) Anyway, here's a few comments from me:

It uses sliding weighted average algo for calculating smoother curve.

@justmara Not that the details matter much, but it's a hybrid of first-order and second-order exponential smoothing functions - so not a true moving average. Basically, first-order exp. smoothing is faster to respond, but less robust (more prone to accepting sudden jumps). Second-order smoothing is slower to respond, but more robust as it assumes that the incoming data follows a trend. I made a hybrid out of the two as for our purposes, we want both: Speed and robustness. Of course, you can't have both at the same time, it's a trade-off. So what the code delivers is basically a little bit of both.

well smoothing actual value leads to slowing down reaction to bg changes which not happen when backsmoothing I like the idea of smoothing bg but i'd prefer backsmoothing jumpy values are only one of the reason of disabling SMB always. we could enable it for some bg sources but not for Libre 2 ( for example) were the issue is producing wrong values instead of error state

@MilosKozak I am often affected by noisy sensors (Dexcom G6) and back smoothing would mean trusting incoming values more than the sum of historic data. As you say, back smoothing will allow for faster reaction by AAPS, but this also means that dosing decisions may fluctuate a bit more when the sensor is noisy. Instead of questioning the current value, back smoothing will make historic data fit to the latest reading, regardless of its plausibility.
The present code might be marginally slower to react to BG changes, but it is better at differentiating between true rises/drops and mere noise. When I wrote it, I wanted some sort of quality control for incoming values, which the current model offers, but ultimately it comes down to taste, probably. I'm open to evolving the code further to allow users to switch on back smoothing, though that may require another addition to the database for smooth transitions.
Btw: I do have testers who specifically use this code because of issues with noisy FSL2 sensors. :P

The effect on local extreme values is not clear to me. At 19:25 smoothed value is lower, at 20:15 higher than raw. Which one is right is hard to tell.

19:30-20:00 seems to be a fake low for me, but this smoothing is not intended to detect or correct it.

@szantos That's the dilemma here. :) The sensor spits out readings, but unless we compare it to true BG measurements (finger pricking..), we have no way to tell whether or not the reading is legit or not. What we can do (and what the code is doing) is a plausibility check. If there's a sudden jump, say +25 mg/dl, then it will mostly be smoothed out unless the trend continues. If the sensor spits out a series of wrong readings (e.g. compression lows at night), then it's very hard to tell if it's legit or not as this requires interpretation and putting the readings into context.

And... is it correct that only the latest 10 readings will become smoothed?

@khskekec Nope. The smoothing begins with historic data, and then progresses into the future until it reaches the latest reading. That's the main difference to back smoothing approaches, where we'd start with the most recent value and update the historic values only, moving backward.
The point of this approach is to update the more recent values as new data becomes available and to have a base against which the most recent value will be quality checked (e.g.: if the last values have been constant, and the most recent reading has a delta of +25 mg/dl, then this change is considered too sudden and extreme to be real, so the delta will be weakened a lot to fit to the previous data). Any data that's older than 10 readings will still be available as smoothed data, but it will no longer be updated. The smoothing algorithm comes with an additional column in the glucoseValue.kt database, so the smoothed data will always be available next to the raw sensor data. If you switch the smoothing on and off in the settings, you will see that the glucose graph on the main screen will update immediately without needing to recalculate the smoothed values.

It was never a requirement for using SMB. The original reason for preferring Dexcom was that the Dexcom data had a noise indicator in it (it still does) so OpenAPS could moderate it's behaviour based on that noise indication.

@tim2000s While true, this is generally not enough for safe SMB delivery. Without smoothing, I've had some completely unnecessary SMBs in response to sensor noise on the G6. I know that BYODA is a smoothed Dexcom source, but AAPS will not reject alternative sources such as xDrip and allow enabling SMB always. Don't get me wrong, I'm a big supporter of xDrip, and because I do not want to use BYODA, I wrote this smoothing plugin.

Hi! Could you please update / rebase the PR? Milos is save of nitpicks done with moving things around.

@Der-Schubi There's been some massive changes to the AAPS code in the last few days that also affect the databases. I'll wait a bit and merge the changes into the smoothing algorithm later when it seems like the biggest changes are over. That's unfortunately not super simple and just getting the database migration right gave me a headache recently. :P But it will get done, eventually.

Those interested in this and other developments by me (Tsunami project) are welcome to join my discord server for discussion. I'm less likely to miss messages there than I am here. ;P https://discord.gg/HFHVd3QB

@justmara
Copy link
Author

justmara commented Nov 26, 2022

@piecycle hey! Didn't know how tag you here :). But this feature is so helpful, and we were talking with you about publishing it, so I decided to try bringing it here by myself. Damn, now it looks like if I've stolen it. You weren't very active in discord lately so I thought you don't have enough time, so I decided to make a move by myself since we were discussing it and already and you weren't against this. This code already was merged to my repo for pretty long time and I've heard many positive feedback from people using it, so decided to share. It is really very helpful feature.
Thank you for it (and for clarification, sorry for calling it simple moving average, didn't mean to offend you (nor code), but simplify things a bit :)

Will you handle updates by yourself then (making new pr or?)

@mxsrm
Copy link

mxsrm commented Dec 4, 2022

How does this PR handle new sensors (I'm using G6 atm, soon G7)? I noticed when I started a new sensor and the first readings came in AAPS with smoothing enabled just showed missing data all the time (waited for 30 minutes so 6 readings). Disabling smoothing, waiting for sensor readings and then re-enabling smoothing worked fine.

@piecycle
Copy link

piecycle commented Dec 4, 2022

How does this PR handle new sensors (I'm using G6 atm, soon G7)? I noticed when I started a new sensor and the first readings came in AAPS with smoothing enabled just showed missing data all the time (waited for 30 minutes so 6 readings). Disabling smoothing, waiting for sensor readings and then re-enabling smoothing worked fine.

You should always see new data immediately. The first few readings will not be smoothed as there's too few reference values, but you should still see something. I'm using a G6 myself with xDrip as my datasource. What is your source, are you using BYODA by any chance?

@mxsrm
Copy link

mxsrm commented Dec 4, 2022

Yes, BYODA.

xDrip displayed the new data (coming from BYOAD) instantly and without problem, so we can rule out sensor or transmitter errors as reason.

@blaqone
Copy link

blaqone commented Dec 4, 2022

Yes, BYODA.

xDrip displayed the new data (coming from BYOAD) instantly and without problem, so we can rule out sensor or transmitter errors as reason.

G7 or g6?

@mxsrm
Copy link

mxsrm commented Dec 4, 2022

At the moment G6.

@rICTx-T1D
Copy link
Contributor

Hope someone can merge it quickly to dev (and later to master). My G6 with last BYODA has some noise too, when sensor is on last day/hours.

@blaqone
Copy link

blaqone commented Dec 5, 2022

Hope someone can merge it quickly to dev (and later to master). My G6 with last BYODA has some noise too, when sensor is on last day/hours.

https://github.com/DiaKEM/dexcom-g7-aaps/tree/blaqone-final

With autoIFS thou.

@MilosKozak MilosKozak mentioned this pull request Dec 8, 2022
@mxsrm
Copy link

mxsrm commented Dec 13, 2022

Yes, BYODA.
xDrip displayed the new data (coming from BYOAD) instantly and without problem, so we can rule out sensor or transmitter errors as reason.

G7 or g6?

This problem also happens when you disable the smoothing after changing the sensor but before the 2hr wait time is over.

Edit: Again, once there a five values available it backfills them and starts working as it should.

@MilosKozak MilosKozak closed this Dec 26, 2022
@tim2000s
Copy link
Contributor

tim2000s commented Jan 16, 2023

I'm not against smoothing per se, but the elephant in the room here is one of the reasons that Dexcom took it out. From what I understand this was because their integration partners requested removal and the original data, not the backsmoothed data.

Whilst it probably doesn't change much, it might be sensible to try to understand why that was.

@piecycle it turns out that the noise data was never used in AAPS, which is why jumpy data for SMB will always have been an issue.

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.