Use MPFR in knumber_float in order to improve floating-point precision
ClosedPublic

Authored by marcelomariano on May 30 2019, 2:55 AM.

Details

Summary

BUG: 132158
BUG: 148357
BUG: 407318
FIXED-IN: 19.08.0

Diff Detail

Repository
R353 KCalc
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
marcelomariano created this revision.May 30 2019, 2:55 AM
Restricted Application added a subscriber: kde-utils-devel. · View Herald TranscriptMay 30 2019, 2:55 AM
marcelomariano requested review of this revision.May 30 2019, 2:55 AM

Adding Evan since he knows best what prevented the use of MPFR when the code was initially added.

CMakeLists.txt
63

Why required? The code is full of #ifdef KNUMBER_USE_MPFR, so it could be made to work in either case.

CMakeLists.txt
63

Sorry, I did not pay attention to this.
In this case I can make MPFR optional and add -DKNUMBER_USE_MPFR when MPFR_FOUND is set.

Mark MPFR as recommended

teran added a comment.May 30 2019, 4:22 PM

Adding Evan since he knows best what prevented the use of MPFR when the code was initially added.

Thanks.

Honestly, it was disabled by default just because it was new and I didn't want any difference in behavior seemingly out of nowhere. The code is probably pretty good, though i expect some regression tests will have different results due to the difference in precision.

Adding Evan since he knows best what prevented the use of MPFR when the code was initially added.

Thanks.

Honestly, it was disabled by default just because it was new and I didn't want any difference in behavior seemingly out of nowhere. The code is probably pretty good, though i expect some regression tests will have different results due to the difference in precision.

I'm attaching the output generated by knumbertest.
Is there any other tests I can run to ensure it's all ok?

cfeck added inline comments.May 30 2019, 5:54 PM
config-kcalc.h.cmake
28 ↗(On Diff #58881)

This is already present in line 20.

knumber/knumber.cpp
211–212

I had to increase it by one more binary digit to get ln(e) == 1 (tested with 20 and 30 decimal digits).

marcelomariano updated this revision to Diff 58959.EditedMay 31 2019, 4:41 PM

Remove duplicated #define HAVE_LONG_DOUBLE 1 in config-kcalc.h.cmake

marcelomariano marked an inline comment as done.May 31 2019, 11:13 PM
marcelomariano marked 2 inline comments as done.May 31 2019, 11:15 PM

Fix binary precision

marcelomariano marked an inline comment as done.Jun 4 2019, 3:09 AM
cfeck added a comment.EditedJun 4 2019, 10:35 AM

Evan, any objections? I would like to commit it to master branch.

Marcelo, please add a comment to bug 407318 with a link to this phabricator page. The mail address is used for the author attribution.

Evan, any objections? I would like to commit it to master branch.

Marcelo, please add a comment to bug 407318 with a link to this phabricator page. The mail address is used for the author attribution.

Done

@cfeck any objections here? I'll wait a few days for an outcome.

Remove GMP from knumber_float implementation and mark MPFR as required.

Update CMakeLists.txt

cfeck accepted this revision.Jun 23 2019, 10:13 AM
This revision is now accepted and ready to land.Jun 23 2019, 10:13 AM
cfeck edited the summary of this revision. (Show Details)Jun 23 2019, 10:15 AM
This revision was automatically updated to reflect the committed changes.