BUG: Fix rollover handling in json encoding #15865
Conversation
This is a fix attempt for issue pandas-dev#15716 as well as pandas-dev#15864. Note that whenever the frac is incremented, there is a chance that its value may hit the value of pow10.
|
please add tests for the issue you are fixing. |
|
please run |
Codecov Report
@@ Coverage Diff @@
## master #15865 +/- ##
==========================================
- Coverage 90.98% 90.97% -0.02%
==========================================
Files 143 143
Lines 49449 49449
==========================================
- Hits 44993 44984 -9
- Misses 4456 4465 +9
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #15865 +/- ##
==========================================
- Coverage 90.98% 90.97% -0.02%
==========================================
Files 143 143
Lines 49449 49479 +30
==========================================
+ Hits 44993 45012 +19
- Misses 4456 4467 +11
Continue to review full report at Codecov.
|
|
Please take a look at the new commits where compliance to cpplinter is considered and tests are added. If the commits are OK, I will try to squash them into one as requested in documentation. Since I am rather green in terms of contributing to open-source project, thanks for being patient with me. |
|
can you also add the examples from both issues (you can put in test_pandas) |
|
you don't need to squash, will do that on merging. also pls add a whatsnew note (bug fix section / I/O), you can list both issues. |
|
Whatsnew entry and more tests are added. Thanks for your kind guidance. |
|
thanks @funnycrab very nice PR! keep em coming! |
|
Thank you! This is my pleasure. Will try to keep'em coming. : > |
|
we have daily dev builds that also test 32-bit. see here, these are on linux. here is an 3.6/32 that fails a couple of tests: https://travis-ci.org/MacPython/pandas-wheels/jobs/218273011 I don't have an easy way to actually test / debug this. but any thoughts on how to fix? welcome to have you push a PR (once it passes main pandas we can merge and 'see' if it passes here). |
|
After check with this and that, if I understand correctly, the test fails on both 32-bit version of Python (2.7 and 3.6). And in both cases, the operating systems are all 64-bit version of Linux, right? My hunch is this has something to do with the floating-point operation, maybe the conversion of Decimal object to float object is handled differently in 32-bit and 64-bit version of Python. Though I have basic understanding in how floating point is typically handled in memory and the relevant IEEE 754 standard, I have never gone this far in Python. I am willing to try to find the reason, just may I ask how I can reproduce this bug first? I have a machine running Ubuntu 16.04 64-bit OS and anaconda Python. But it seems I failed to install 32-bit version Python environment with anaconda. I followed this solution in the attempt. |
|
you need to install 32-bit linux (which is not that common, but for now we support it). you can use the docker image thats in the repo (or I image some are out there find via google). There might be a free web service out there were you can just get a virtual box already setup for this lmk - we can skip these tests, but ideally like to figure it out. |
|
I managed to set up a AWS instance to reproduce the bug and the following are my findings. Say we take the failed test case as example (actually all the newly added test cases should fail on 32-bit Linux), pd.DataFrame([dict(a_float=0.95)]).to_json(double_precision=1)On 32-bit Linux, the intermediate variables around here have the following values , ...
pow10 = g_pow10[enc->doublePrecision];
whole = (unsigned long long)value;
tmp = (value - whole) * pow10;
frac = (unsigned long long)(tmp);
diff = tmp - frac;
// output 0.9499999999999999555910790149937383830547
printf("value-whole is %.40f\n", value-whole);
// output 9.5000000000000000000000000000000000000000
printf("value*pow10 is %.40f\n", value*pow10);
// output 0.5000000000000000000000000000000000000000
printf("value*pow10-frac is %.40f\n", value*pow10-frac);
// output 0.4999999999999995559107901499373838305473
printf("(value-whole)*pow10-frac is %.40f\n", (value-whole)*pow10-frac);
// output 9.5000000000000000000000000000000000000000
printf("(value - whole)*pow10 is %.40f\n", (value-whole)*pow10);
// output 0.9499999999999999555910790149937383830547
printf("value %.40f\n", value);
// output 10.00
printf("pow10 %.2f\n", pow10);
// output 0
printf("whole %llu\n", whole);
// output 9.5000000000000000000000000000000000000000
printf("tmp %.40f\n", tmp);
// output 9
printf("frac %llu\n", frac);
// output 9.0000000000000000000000000000000000000000
printf("frac (convert to double) %.40f\n", (double)frac);
// output 0.4999999999999995559107901499373838305473
printf("diff %.40f\n", diff);
if (diff > 0.5) {
++frac;
} else if (diff == 0.5 && ((frac == 0) || (frac & 1))) {
/* if halfway, round up if odd, OR
if last digit is 0. That last part is strange */
++frac;
}
...However, on 64-bit Linux, ...
pow10 = g_pow10[enc->doublePrecision];
whole = (unsigned long long)value;
tmp = (value - whole) * pow10;
frac = (unsigned long long)(tmp);
diff = tmp - frac;
// output 0.9499999999999999555910790149937383830547
printf("value-whole is %.40f\n", value-whole);
// output 9.5000000000000000000000000000000000000000
printf("value*pow10 is %.40f\n", value*pow10);
// output 0.5000000000000000000000000000000000000000
printf("value*pow10-frac is %.40f\n", value*pow10-frac);
// output 0.5000000000000000000000000000000000000000
printf("(value-whole)*pow10-frac is %.40f\n", (value-whole)*pow10-frac);
// output 9.5000000000000000000000000000000000000000
printf("(value - whole)*pow10 is %.40f\n", (value-whole)*pow10);
// output 0.9499999999999999555910790149937383830547
printf("value %.40f\n", value);
// output 10.00
printf("pow10 %.2f\n", pow10);
// output 0
printf("whole %llu\n", whole);
// output 9.5000000000000000000000000000000000000000
printf("tmp %.40f\n", tmp);
// output 9
printf("frac %llu\n", frac);
// output 9.0000000000000000000000000000000000000000
printf("frac (convert to double) %.40f\n", (double)frac);
// output 0.5000000000000000000000000000000000000000
printf("diff %.40f\n", diff);
if (diff > 0.5) {
++frac;
} else if (diff == 0.5 && ((frac == 0) || (frac & 1))) {
/* if halfway, round up if odd, OR
if last digit is 0. That last part is strange */
++frac;
}
...And I would like to bring your attention to the following two lines of code and their outputs, // output 0.5000000000000000000000000000000000000000
printf("value*pow10-frac is %.40f\n", value*pow10-frac);
// output 0.4999999999999995559107901499373838305473
printf("(value-whole)*pow10-frac is %.40f\n", (value-whole)*pow10-frac);64-bit Linux // output 0.5000000000000000000000000000000000000000
printf("value*pow10-frac is %.40f\n", value*pow10-frac);
// output 0.5000000000000000000000000000000000000000
printf("(value-whole)*pow10-frac is %.40f\n", (value-whole)*pow10-frac);Since whole is 0 in this case, mathematically, expression value*pow10-frac and (value-whole)*pow10-frac should yield exactly the same result which is not the case on 32-bit Linux. In this sense, the result produced on 32-bit Linux is kind of strange, maybe the compiler does some optimization which change the order of the evaluation and thus different rounding? Also, even on 64-bit Linux, the two expressions give the same result, I find it difficult to understand. Since the original value is 0.9499999999999999555910790149937383830547, based on what rule does the code know that it should round the value of expression value*pow10 to 9.5000000000000000000000000000000000000000 instead of 9.4999999999999995559107901499373838305470. Maybe it is due to the fact that the significand part has reached some upper limit? Since I don't know the exact reason, I choose not to temper with the code before you kindly shed some light on this. Thank you! |
|
so maybe some casting e.g. |
|
Yes, Tried following, no luck. // output
// (value - (double)(whole))*pow10-frac is 0.4999999999999995559107901499373838305473
printf("(value - (double)(whole))*pow10-frac is %.40f\n", (value-(double)(whole))*pow10-frac);And I believe when the data types of the two operands are different, the implicit cast should be done automatically. Also, I checked the size of the unsigned long long and double on 32-bit Linux, quite normal // output
// size of unsigned long long is 8
printf("size of unsigned long long is %d\n", sizeof(unsigned long long));
// output
// size of double is 8
printf("size of double is %d\n", sizeof(double)); |
|
skipping these. (please feel free to keep digging if you want). though more interested if you want to solve other issues :> |
This is a fix attempt for issue #15716 as well as #15864.
Note that whenever the frac is incremented, there is a chance that its
value may hit the value of pow10.