-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix MMOLL_TO_MGDL conversion constant. #1737
Fix MMOLL_TO_MGDL conversion constant. #1737
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we could be more precise with 180.15588 g/mol the suggested change should be precise enough.
Would you please explain how this is a 0.15% error? (180.182 - 180.156) / ((180.182 + 180.156)/2) = 0.014% This seems to be really unnecessary. |
Ah right, it's a 0.0144% error, sorry. Will fix.
I agree that the impact is very minor, but compared to the kind of effort we'd invest to fix, say, a consistent 1% error, it's fairly high impact per effort, wouldn't you agree? |
Stupid question, what is "LL"? I'm all in favor of fixing other places that also get this wrong...as it stands, some diabetes software uses 18.0182 and some uses 18.0156. Surely everyone should all agree on one value, and then it makes sense to pick the value that correctly represents the units that we claim we are using, no? |
Please note: If the glucose reading from the Dexcom app is 400, which is the highest value xDrip recognizes, it will be translated currently to (rounded to 4 decimal points: This is an error of 0.0032mmol/L. If I ask you what is the ratio of the perimeter of a circle to its diameter, what exactly will you answer me with? I would open a PR suggesting to the developers to increase the number of decimal points to 2 if I was really concerned about accuracy because that is the bottleneck not the error you are talking about here. However, that is not a good idea either for a different reason. Even most blood glucose meters make the same rounding. |
I know, and that is why my commit message said that rounding to a whole mg/dl (or a 1/10th mmol/l) introduces a larger error. If the value read from the Dexcom app is an integer, then that is the biggest problem here. The Dexcom app should report a value in the highest precision that it can reasonable do. Only after all calculations (e.g. conversion to mmol/l are done) are done and the value is displayed in a user interface should it be rounded to a number of digits that somewhat represents the accuracy, to not needlessly overload the human which bogus precision that could be misinterpreted as accuracy.
The distinction that your argument is missing is that the 1 decimal point figure is read by a human. It's distracting to a human to have pointless additional digits present. Presenting another digit would impose a huge burden on all the humans who are reading these measurements every day. Until the value is presented in a user interface, however, this is not the case; it's no different to a computer to calculate with one or the other value.
It depends on whether the result is then immediately used by a human, or flows into some numeric computation where errors accumulate. I sure hope that if I write Math.PI in my Java code then the double value that I get is the double value that is closest possible to the true value of pi. Anything else is objectively wrong. I also want to point out that the previous constant value that this commit is changing is wrong not in a matter of presenting too few digits. It's not a rounding error. It's objectively just wrong. tl;dr I agree with you that the extra precision would be pointless or even detrimental in the user interface. I disagree that it is pointless in internal calculations.
Re wasting your time, I observe that you are investing a lot of effort defending a constant value that is objectively wrong.
No, that would be harmful because that would impose extra load on the humans reading the numbers. Numbers presented in a user interface must be reasonably rounded. Numbers used for internal calculations must not do needless rounding before the number is presented in a UI, because those rounding errors might accumulate.
They make them in the user interface. If Dexcom does the same rounding (e.g. to an integer number of mg/dl) in its API, then I consider this a problem worth fixing, although one outside the scope of this pull request. |
You have made good points. Do you have any references? |
Please increase accuracy to 3.14159265, and don't hesitate to waste time for writing and reading nonsense! |
Is this a joke to you? |
tl;dr I'm fine with either constant potentially for consistency with other apps, but if we keep the current one then I think it should carry a comment pointing out that the value is wrong and only used for consistency with [list of other apps], so that if someone copies the value from xDrip+ one day they at least know that the value is wrong. @gui-dos mentioned "LL" as using the same factor, but I don't know what that is. What is your preference? Somewhat pointlessly :), I'll elaborate a bit further below.
References:
Re. intentional, the main argument against the change that I can see is that the unit is arbitrary anyway. Instead of mmol/l, it's just as valid to use mg/dl, or mmol per pint, or stone per pint, or whatever, as long as the unit is consistent. I wish everyone agreed on mmol/l vs. mg/dl, too. Being consistent is most important. If everyone used 18.0182, then xDrip+ should, too (but we shouldn't call that unit mmol/l). Unfortunately diabetes software is not consistent and probably never will be (we probably will also never agree on mg/dl vs. mmol/l). I've found 18.0182, 18.0156, and 18.0 (by the Dexcom G6 app, shame on them) all used. I haven't done a survey of whether the apps that xDrip+ interfaces with more commonly use one or the other constants. It does feel like the value that's consistent with the unit "mmol/l" is objectively correct though, so it feels like the chance of getting everyone to agree on that value might be higher (it's still not easy, as this discussion thread here shows). I can't think of any reason why the molar mass of glucose in the blood would be different from other environments. My personal theory of what happened is that someone looked up the (rounded) molar mass of carbon, hydrogen, and oxygen somewhere, and then tallied them up (6x carbon + 12x hydrogen + 6x oxygen), accumulating round error; or maybe someone who copied the value by hand accidentally repeated the digits 18; or maybe one value is from before those masses were more precisely known. But I don't know for sure that either of these is what happened. I obviously don't know for sure that the 18.0182 value is not intentional. I also find it not unbelievable though that the value might be wrong even if it was widely used, because everyone in science just copies from others. One story that I've read, and find credible, is that tuberculosis is airborne and it's carried in airborne droplets of up to 5 micrometers in size. At some point, this was misrepresented/misunderstood as saying that 5 microns is the limit at which infectious particles will be airborne (as opposed to quickly dropping to the ground), when the true value is closer to 100 microns (if I remember the numbers right). This led to early misguidance on coronavirus because the droplets it's contained in tend to be larger than than 5 microns and it was therefore initially assumed (in spite of better evidence, e.g. common infections across a restaurant or airplane!) that most of the infectious particles would not remain airborne. If the story is right (I obviously haven't checked the underlying details myself), then misjudgements derived from that error probably caused needless deaths. |
PS. I also did a Google search for
to see if there might be an existing discussion or controversy, but sadly got no results. |
Maybe we can close this with a compromise? I suggest 18.017. |
Interesting. http://www.endmemo.com/medical/unitconvert/Glucose.php even claims 18.01801801801802 (which looks awfully like repeating, i.e. precisely 18000/999, which would obviously be bogus). Perhaps 18.018 is indeed more commonly used. https://www.google.com/search?q=%2218.0156%22+mmol%2Fl+mg%2Fdl finds results, though. How about I abandon this pull request and upload a different one that instead adds a comment cautioning against copying the value elsewhere and pointing to this discussion thread? |
@tzachi-dar Using the Google term frequency as a reference is quite dangerous because it does not mean a term or value is correct. I recommend to reference public sources like PubChem. Even if the error is small and has no measurable effect, we should use the correct value and not the one which is the most frequently and wrongly used value. Just another reference: Molar Mass Of Glucose (C₆H₁₂O₆). |
If I was the person who had to make the final decision to merge this or not, I would want to know why we are were we are today, meaning why did this happen to begin with. |
@Navid200 If it doesn't matter, then why are you investing this much energy in defending the old (incorrect) value? I want to point out that this PR was already approved before you jumped in. Regarding the rounding performed in the UI being a much bigger source of error, please refer to my earlier response.
With respect, this is a terrible algorithm to use to determine whether or not to merge something:
I would accept an argument based on the wrong value being a de facto standard because it's more commonly used, but that's not the argument you're making. The biggest damage however is that your insistence on something that you claim to not matter demonstrates a kind of gatekeeping attitude that does damage to the goal of attracting new talent towards maintaining this codebase. The codebase urgently needs this talent - as evidenced by the fact that the app targets an 8 year old version of Android, there's code duplication (this commit touches two different Constants.java), lots of dead code, and insufficient unit test coverage (note how this commit didn't have to touch any test code?). Also, the appwidget could be improved to be much easier to read / more informative. I find this kind of attitude really harmful to the project. I was all keen to contribute and figured I'd test the waters with a tiny commit that is objectively right and should cause minimal discussion. Congratulations on making this project so unwelcoming to have destroyed any motivation that I had towards making bigger improvements. |
I only assumed that you were someone who cared about having a healthy discussion: everyone's opinion is welcome. This project has been going on for years and is currently perfectly healthy. I will do my best to keep it going. There are occasionally a few individuals who have no idea what the objective is and claim that they are very talented and now are going to leave, not realizing that they are just showing they don't understand the main objective. The main objective is to have a product that continues to serve the users. The users who are using old technology. The ones who are using newer technology. All of those devices need to stay functional. Every single proposal has to go through a rigorous scrutiny to ensure that there is absolutely no chance of breaking the functionality for anyone. Talent is great. A talent that lacks understanding of the main objective is useless. I have open PRs. The last thing I want to happen is to have one of them go through and later on discover that it broke someone's system. I'm not sure why you are complaining. I wasn't rude. I stated my opinion. I also said I am not making the final decision. |
I was interested in having a healthy discussion. Your previous post where you showed with a bunch of graphs why this change in your view doesn't matter, and then presented a decision algorithm that was so heavily biased towards the status quo as to stick to the status quo even when that status quo is objectively wrong, however, was not a healthy discussion. We had also already discussed the fact that the rounding performed in the UI introduces a much bigger problem (this is correct, but unlike this bug here it is due to a limitation in humans and thus unavoidable).
The point I presented in my last comment laid out several pieces of evidence on how the project is currently NOT as healthy as it could/should be, and further sketched how the way this discussion has gone is contributing to this problem. It doesn't matter whether I am talented or not because I'm just one person, but for the project as a whole, attracting this talent seems like a pretty important ingredient towards having a product that continues to serve its users (which you say is the objective). For example, the fact that the app targets an 8 year old API version with the corresponding comment only providing a very vague justification will become a huge burden for the project whenever some version of Android changes something that does require the targetSdkVersion to advance.
I agree that that would be terrible. Another thing I had considered as my first PR was to add better test coverage - perhaps I should have gone with that instead. However, being too scared to touch anything just because the original reason isn't clear doesn't lead to a long-term healthy codebase; it leads to a local maximum that one cannot escape from, until the project eventually becomes unsalvagable. This particular failure case of projects is one that lots of people way smarter than me have written books and blog posts about (it's often referred to as "Fear driven development"). Had you observed that this PR doesn't touch any unit tests and asked me to add unit test coverage so that future changes in this constant are observed, I would have happily obliged. That would IMHO have been a healthier way to engage with the failure that already happened, namely that the codebase contained the wrong physical constant, that there was code duplication, and that no unit tests covered this area. |
The molecular weight of glucose we are currently using is indeed wrong. That's not a matter of a |
I don't know how else to explain that I have no authority to stop or approve your, or anyone else's, PR other than what I have already stated multiple times. If you have a good suggestion, you should not be bothered by questions and opinions. If someone presents an irrelevant opinion, you can just state your view, and carry on with your objectives. I will continue to participate in discussions. If that is a negative for this project, I will stop. However, I don't believe that is the case. I need to be told not to do it before I stop. If you become a member and there is a proposal you have questions about, my advice to you is to ask questions, and present your opinion even if is in disagreement with someone else. |
app/src/main/java/com/eveningoutpost/dexdrip/UtilityModels/Constants.java
Show resolved
Hide resolved
wear/src/main/java/com/eveningoutpost/dexdrip/UtilityModels/Constants.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please insert the comment for clarification.
I never disagreed with what the correct weight was. Calling a discussion not healthy because you disagree with the other side is not very healthy either. |
Really sorry, my mistake. I meant medical discussion, not healthy discussion. I still have to learn common English. |
The molecular weight of glucose is 180.156 g/mol, so (glucose mmol/l) / (glucose mg/dl) = 180.156g * dl/l = 18.0156, rather than 18.0182. Note: xDrip+ is not the only Diabetes-related software that has the (as far as I can tell) wrong constant. Several other software packages also have the value 18.0182 that I believe to be incorrect, while others have 18.0156 (correct); I suspect that someone got it wrong at some point and the others just copied from them. Dexcom's G6 app uses the value 18.0 in at least one place, which is even further off. While this seems like an unnecessary place to introduce error, the value was < 0.015% off the correct value. Rounding to whole mg/dl or to 1/10th of a mmol/l introduces a slightly larger error than this, so if one rounds both before and after the conversion then in practice, the result is unaffected by the exact value of the constant.
bfd3afd
to
14b1159
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though this PR introduces a small change with minimal mathematical impact, it corrects a constant that we have been using incorrectly for a long time. Hence, I suggest merging it.
All you need to do is to control your emotions. |
I agree with you. That's why I was surprised you invested so much energy into fighting this PR. I originally picked this as my first PR because I expected it to be uncontroversial. |
Needs no energy at all. |
No need to comment and not much use to comment this too long exchange of opinions and questions, but: |
Just to eliminate one theory for where the number 18.0182 might have come from, I looked into whether the molar mass of glucose in living things might be sufficiently high to explain this; tl;dr I don't thing the numbers add up, so I still think that the number is most likely just a typo that has since been kept alive by a mix of mindless copying and, as seen in this thread, Chesterton's Fence). There are different isotopes of the elements. Biomatter has a higher proportion of 14C, so the molar mass of carbon in very slightly higher than the conventional 12.011g/mol for Carbon (that's what carbon dating is based on). That's why the standard atomic weight of Carbon is published by IUPAC as an interval, [12.0096, 12.0116] g/mol. For oxygen, it's [15.99903, 15.99977] Even calculating with the top ends of those ranges, glucose = C6H12O6 should have a molar mass <= 6 * 12.0116 + 12 * 1.00811 + 6 * 15.99977 = 180.16554. Mind you, I suspect that this number is higher than the true molar mass of D-Glucose in humans, but even so it accounts for not even half of the difference (180.156 generally accepted molar value of glucose vs. 180.182 implied by the constant in the code). My understanding (not sure) is that the standard atomic weight range covers biological samples, so this seems to imply that the molar mass of glucose in living things, while higher (because of more C14) than in dead things, is not enough to account for the difference. My suspicion is still that the number is just a typo that has spread unchecked. I disagree that "we don't know where the number came from" is sufficient reason to stick with the current number. To me, the real issues here are that
All of these issues have already occurred. Pushing back on this PR without providing a justification for the old value just means that these issue cannot be fixed. In contrast, my commit fixes the first of these issues, because at least it comes with a justification that is potentially falsifiable. That said, I'm also open to leaving the old value in place, but I feel that that is only reasonable if it is held to the same standard, i.e. an explanation for why that value is kept is added. Without that, sloppy commits are held to a lower standard than non-sloppy ones, and I think it's clear what kind of codebase that model leads to. (And of course, the argument that the change makes no difference is weird, because if this was really one's motivation then one wouldn't bother constructing a bunch of graphs to show that yes, as we already know, the difference is quite small). |
I fortunately haven't been involved in the previous conversation and debate. As far as I understand it, changing this constant wont make any operational difference to anything users see. Correct me if I have misread the above. That being the case, it might be scientifically correct, which is no bad thing, but it seems a waste of time to have to consider the change. |
I don't know, it depends on how the constant is used. My engineering philosphy is that the constant is so fundamental and affects so many people that it should be correct. Something far removed (e.g. an insulin pump, or statistics over a long time horizon) should still be able to trust the constant to be correct. It's not like fixing it is a lot of work (only the discussion was work).
I am surprised myself by the amount of debate about something that to me appeared uncontroversial. |
The molecular weight of glucose is 180.156 g/mol, so (glucose mmol/l) / (glucose mg/dl) = 180.156g * dl/l = 18.0156, rather than 18.0182.
Note: xDrip+ is not the only Diabetes-related software that has the (as far as I can tell) wrong constant. Several other software packages also have the value 18.0182 that I believe to be incorrect, while others have 18.0156 (correct); I suspect that someone got it wrong at some point and the others just copied from them. Dexcom's G6 app uses the value 18.0 in at least one place, which is even further off.
While this seems like an unnecessary place to introduce error, the value was less than 0.15% off the correct value. Rounding to whole mg/dl or to 1/10th of a mmol/l introduces a slightly larger error than this, so if one rounds both before and after the conversion then in practice, the result is unaffected by the exact value of the constant.