StephenBlackWasAlreadyTaken / xDrip-Experimental

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

BgReading.find_new_curve, division by zero ? #365

Open JohanDegraeve opened 8 years ago

JohanDegraeve commented 8 years ago

Hi,

while porting the Android code to actionscript, I'm getting a division by zero error.

on this place : https://github.com/StephenBlackWasAlreadyTaken/xDrip-Experimental/blob/master/app/src/main/java/com/eveningoutpost/dexdrip/Models/BgReading.java#L691 find_new_curve gets called during initial calibration. if there's only two readings stored. This method gets called for each reading. Which means x2 will be the timestamp of the reading for which the method is called, x1 will in one the cases be the timestamp of the reading read from the database, which has exactly the same value, which results in x1 - x2 being 0, and a division by zero error.

That's what happens in my code : https://github.com/JohanDegraeve/iosxdripreader/blob/master/src/databaseclasses/BgReading.as#L513

in actionscript this gives "-Infinity" and the code just continues, but I would assume in Java this would throw an exception ? (I could debug on android studio but this thing is so slow - if no one knows the answer I'll have to do it anyway)

thanks

Johan

AdrianLxM commented 8 years ago

I don't see why x1 and x2 are the same. They should be 5 minutes apart.

AdrianLxM commented 8 years ago

Both values are read from the DB: https://github.com/StephenBlackWasAlreadyTaken/xDrip-Experimental/blob/master/app/src/main/java/com/eveningoutpost/dexdrip/Models/BgReading.java#L658

This means they really should be 5 minutes apart. But as both calls query the same db with the same query the result for their curve defined by a,b and c should be the same.

tzachi-dar commented 8 years ago

I second adrian,

On xDrip there is code that checks that there is at least two minutes difference between two packets. This code is in: TransmitterData create().

So for xDrip there should be no issue.

On Wed, Jul 27, 2016 at 2:20 AM, AdrianLxM notifications@github.com wrote:

Both values are read from the DB: https://github.com/StephenBlackWasAlreadyTaken/xDrip-Experimental/blob/master/app/src/main/java/com/eveningoutpost/dexdrip/Models/BgReading.java#L658

This means they really should be 5 minutes apart. But as both calls query the same db with the same query the result for their curve defined by a,b and c should be the same.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/StephenBlackWasAlreadyTaken/xDrip-Experimental/issues/365#issuecomment-235435730, or mute the thread https://github.com/notifications/unsubscribe-auth/AHkw5NqplyPsFQOHD4glfCeMx0DGHD27ks5qZpY2gaJpZM4JVnTp .

JohanDegraeve commented 8 years ago

Ok i'll have to start debugging both versions and compare the results step by step, I'm not saving the bgreadings to database at the same moments as the android app - could be the reason I'll keep you informed

JohanDegraeve commented 8 years ago

was an error in my code

JohanDegraeve commented 8 years ago

sorry, it does seem to occur in the android version. I have a branch here which adds traces and allows me to send them via e-mail (homeview, top right menu has an item "E'-mail logfile" This is the result. xdrip-log-2016-08-04-22-45-19.txt

What I do : install the app, start the sensor (shift back the time with exactly two hours) and wait for minimum two readings, when app says ("calibrate") then I do the double calibration. In BgReading.find_new_curve, the value of second_latest is being printed. On line 219 of the logfile it shows "Infinity" Actually this value has been set in the previous call to find_new_curve. find_new_curve is being called two times in Calibration.initialCalibration. Once for the first calibration, once for the second calibration.

I think actually the error is here : https://github.com/JohanDegraeve/xDrip-Experimental/blob/master/app/src/main/java/com/eveningoutpost/dexdrip/Models/BgReading.java#L684

it says "double x2 = timestamp;" it should be "double x2 = latest.timestamp;"

tzachi-dar commented 8 years ago

It seems that you are right, in your analysis of the problem. The function is called on the two points.

I wonder what will happen once we fix this. It seems to me that the b part only influences the predictive code, which is off by default.

JohanDegraeve commented 8 years ago

I don't understand the algorithms itself - but I think this is a rare case, it only happens when the user shift the start time two hours back, and then calibates immediately after having received two readings. It occurred to me know because I was waiting for the readings to do the tests.

2016-08-05 0:14 GMT+02:00 tzachi-dar notifications@github.com:

It seems that you are right, in your analysis of the problem. The function is called on the two points.

I wonder what will happen once we fix this. It seems to me that the b part only influences the predictive code, which is off by default.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/StephenBlackWasAlreadyTaken/xDrip-Experimental/issues/365#issuecomment-237699766, or mute the thread https://github.com/notifications/unsubscribe-auth/ANMwTfwDUiJjCsd5m3GoS1drbWm9CKM4ks5qcmRdgaJpZM4JVnTp .

tzachi-dar commented 8 years ago

I think that your proposed solution is correct, but I want to think about it some more (and try it) before we move on.

AdrianLxM commented 8 years ago

If I understand this right it is a problem for the first of the two readings, not for the second as for the second latest.timestamp is the same as timestamp. It only occurs if there are just 2 readings.

As far as I remember the curve is used for future prediction. Either to directly show it on the screen or for an arrow symbol. It is always taken from the latest bg reading.

As it is calculated correctly for the second reading and never used for the first it should not be a problem in practice at all. It is still a bug in the code an we should correct it.

JohanDegraeve commented 8 years ago

Indeed only in case there's two readings, and only for one of the two readings it goes wrong. It's already good to know for me how to correct this, because in my case (developping in ActionScript) I was getting an exception when trying to store this reading in the database. The rest of the code was not executed anymore. It's strange that there's no exception in the Android version. (although I didn't check exactly what happens in the debugger)

AdrianLxM commented 8 years ago

Only when there is 2 readings, only for one and only for the one we later never look at again ;)

A division by zero will only lead to an ArithmeticException if it is of int type. The floading point types also can evaluate to infinity or NaN.

tzachi-dar commented 8 years ago

I guess we all agree. I'll make a pr, will test it and commit it once we have some more confidence.

JohanDegraeve commented 8 years ago

ok

2016-08-06 23:25 GMT+02:00 tzachi-dar notifications@github.com:

I guess we all agree. I'll make a pr, will test it and commit it once we have some more confidence.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/StephenBlackWasAlreadyTaken/xDrip-Experimental/issues/365#issuecomment-238049522, or mute the thread https://github.com/notifications/unsubscribe-auth/ANMwTfeX5QacWEY7xFY7deABf53aOov_ks5qdPuvgaJpZM4JVnTp .

tzachi-dar commented 8 years ago

By the way, I think we have a similar bug also for the three packets case. I'm fixing it as well, in my pr, I guess that for the same reasons AdrianLxM mentioned, it shouldn't affect anything.

tzachi-dar commented 8 years ago

Opened PR: https://github.com/StephenBlackWasAlreadyTaken/xDrip-Experimental/pull/373

AdrianLxM commented 8 years ago

@tzachi-dar, good morning. It seems like you have modified find_new_raw_curve but we've talked about find_new_curve here? So we have this bug in both.

find_new_raw_curve directly affects calibrations output. I've checked that save() is called everytime before find_new_raw_curve also -> the modification therefore should not do any changes in behaviour either.

tzachi-dar commented 8 years ago

Thanks for finding that, i'll fix both functions but it will take a few days untill I can do it.

בתאריך 7 באוג׳ 2016 10:59 לפנה״צ,‏ "AdrianLxM" notifications@github.com כתב:

@tzachi-dar https://github.com/tzachi-dar, good morning. It seems like you have modified find_new_raw_curve but we've talked about find_new_curve here? So we have this bug in both.

find_new_raw_curve directly affects calibrations output. I've checked that save() is called everytime before find_new_raw_curve also -> the modification therefore should not do any changes in behaviour either.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/StephenBlackWasAlreadyTaken/xDrip-Experimental/issues/365#issuecomment-238069405, or mute the thread https://github.com/notifications/unsubscribe-auth/AHkw5H9bvYpVH98fyy50py8N8OPvFueNks5qdZBGgaJpZM4JVnTp .

tzachi-dar commented 8 years ago

OK, fixed the other function as well. Will test that and see if there are any problems with that.