search

StephenBlackWasAlreadyTaken / xDrip-Experimental

Experimental Branches for Collaboration on DexDrip
GNU General Public License v3.0
25 stars 62 forks source link

Just adjust last value on calibration. #360

Closed AdrianLxM closed 8 years ago

JohanDegraeve commented 8 years ago

why ?

AdrianLxM commented 8 years ago

@JohanDegraeve, if you haven't followed the discussions on gitter:

There are a few problems with adjusting gradually over the last 30 values that people don't like:

Disabling the adjustment is the simplest solution. For my stats I actually really like the adjustment.

If someone has an idea and the time for a better solution that sure would be great.

jstevensog commented 8 years ago

I hate to say it, as I am happy to have this off, but there are likely people who want it. An option in settings perhaps, with the default off?

---- AdrianLxM wrote ----

@JohanDegraeve, if you haven't followed the discussions on gitter:

There are a few problems with adjusting gradually over the last 30 values that people don't like:

the gradual adjustment will destroy trends. Example: the trend is downwards but in general too low. After a calibration it will be gradually adjusted and look like an upwards trend.unfiltered values are not adjusted -> lines differ (and this is the less time consuming fix)Calibration overrides will leave strange bg patterns behind. Even without overriding calibrations the adjustment intervals can overlap. 30 values can be quite some time with a few missed readings in between.When people have to explain their behaviour to their care team it is good to have the data as it was present at that time.

Disabling the adjustment is the simplest solution. If someone has an idea and the time for a better solution that sure would be great.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

AdrianLxM commented 8 years ago

@jstevensog added settings. Now it is optional with default off. Good idea. 2 a.m. over here - I'll test it tomorrow.

jstevensog commented 8 years ago

Nice work Adrian, I must pull all the latest merges and rebase PebbleTrend. Also need to get the watch face installation in there. Would be good to get it ready for the next release. Cheers

---- AdrianLxM wrote ----

@jstevensog added settings. Now it is optional with default off. Good idea. 2 a.m. over here - I'll test it tomorrow.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

JohanDegraeve commented 8 years ago

@AdrianLxM thanks for the explanation, I don't read all gitter discussions, too much too read :)

AdrianLxM commented 8 years ago

Tested enable/disable in settings and it and works for me.

jamorham commented 8 years ago

Would this have any effect on find_new_raw_curve()

https://github.com/StephenBlackWasAlreadyTaken/xDrip-Experimental/blob/48f08fd795c2d71385dbf4edd507d8cc9372b8ab/app/src/main/java/com/eveningoutpost/dexdrip/Models/Calibration.java#L624 ?

Do we need to worry about the other call to adjustRecentBgReadings from the initial calibration?

https://github.com/StephenBlackWasAlreadyTaken/xDrip-Experimental/blob/48f08fd795c2d71385dbf4edd507d8cc9372b8ab/app/src/main/java/com/eveningoutpost/dexdrip/Models/Calibration.java#L265

tzachi-dar commented 8 years ago

good change, I say merge it.

AdrianLxM commented 8 years ago

@jamorham It has no influence on the initial calibration. The method is not changed just called with a different parameter.

AdrianLxM commented 8 years ago

@jamorham find_new_raw_curve() should also not be influenced as we don't change the raw values that it uses. I guess in general find_new_raw_curve() does not need to be called here as it should give the same results. But as the same function is used for initial calibration, it then will do it for the first time.

The only thing I see it influences is the slope arrow and prediction until the next value comes in. With a jump due to the calibration the slope between those two values can be larger. Shall we adjust for 2 readings - then it would not have any influence?

jamorham commented 8 years ago

@AdrianLxM you can see my confusion regarding find_new_raw_curve() If adjustRecentBgReadings doesn't affect it then why is it called from within that method?? I don't know the answer to the question, I'm really just flagging that there there might be potential for some other consequence of the change.

AdrianLxM commented 8 years ago

@jamorham As I see it, ´find_new_raw_curve()` is just called there as the initial calibration will adjust the values before that one as well - or better generate them. In this case, the call is really needed.

jamorham commented 8 years ago

Okay I think this means that this change would be safe so it looks good to me. At some point maybe find_new_raw_curve() should be moved to make things nicer.