GNOME Bugzilla – Bug 759439
Use MPC instead of MPFR
Last modified: 2017-02-06 18:59:27 UTC
Currently calculator uses MPFR for it's calculations. MPFR only supports real numbers and so there is some extra code in calculator to implement complex numbers on top of this. Unfortunately this code is incomplete - there are an number of functions that are not implemented for complex numbers (bug 759169). Currently calculator does nothing to avoid overflows or loss of precision in intermediate results when calculating complex results. Indeed it does not even use MPFR's hypot() and atan2() to implement abs() and arg(). GMC¹ is a library for multiprecision complex numbers built on top of MPFR. It has the same license and comes from the same stable as MPFR and it is actively maintained. Using GMC would give calculator support for complex numbers for all the trig functions, log, exp, sqrt etc very simply and without worrying about overflows etc. I've had a look at porting it and it shouldn't be too difficult. It should also be possible to improve the MPFR bindings at the same time to make use of vala's automatic memory management. I've CC'd Daniel and Arth as they were involved in the port to MPFR and might be aware of some issues that I've overlooked. ¹http://www.multiprecision.org/index.php?prog=mpc
>GMC¹ is a library for multiprecision complex numbers built on top of MPFR. Sorry that should read MPC¹ is a library for multiprecision complex numbers built on top of MPFR.
Created attachment 317399 [details] [review] Patch for automatic memory management Hey, using MPC seems like a great idea :). When I originally did the port to MPFR, I ran into some issues with automatic memory management, so I chose to use manual memory management. This is pretty bad in retrospect :(. Here is a patch that I created, that changes current git master to use automatic memory management for MPFR. But this definitely needs to be tested a bit before being merged.
Hi guys, I'll be very honest here, even though I'm maintaining GNOME Calculator now, I'm very unfamiliar with the "math" side of things here, with some familiarity with the number.vala class by now though. Daniel, is your patch a pre-requisite or a first in a sieries for moving towards MPC instead of MPFR? Instead it'd be nice to have this patch in a different bug. My proposal is that the work towards porting to MPC is done in a wip/mpc branch on top of master that we can test. But I have very little experience with it so I'm letting you guys lead the effort.
(In reply to Daniel Renninghoff from comment #2) > Created attachment 317399 [details] [review] [review] > Patch for automatic memory management > > Hey, using MPC seems like a great idea :). Thanks for the encouragement, sorry it's taken me so long to reply > Here is a patch that I created, that changes current git master to use > automatic memory management for MPFR. But this definitely needs to be tested > a bit before being merged. That was fast! I've used your updated bindings as a basis for my MPC port, I'm not sure it's worth changing number.vala to use the new MPFR bindings only to immediately switch to MPC though.
(In reply to Alberto Ruiz from comment #3) > Hi guys, > > Daniel, is your patch a pre-requisite or a first in a sieries for moving > towards MPC instead of MPFR? Instead it'd be nice to have this patch in a > different bug. Speaking for my self I think it would be simpler to update the MPFR bindings in the same commit as the switch to MPC to reduce the amount of code churn. (As MPC uses MPFR we still need the bindings for a few pieces of the API) There doesn't seem to be much point to rewriting and reviewing number.vala to use MPFR with automatic memory management and then rewrite it again to use MPC. > My proposal is that the work towards porting to MPC is done in a wip/mpc > branch on top of master that we can test. But I have very little experience > with it so I'm letting you guys lead the effort. I've made some progress this week - the tests pass and it seems to work. I'll clean things up and push something to wip/mpc sometime next week.
(In reply to Phillip Wood from comment #5) > I've made some progress this week - the tests pass and it seems to work. > I'll clean things up and push something to wip/mpc sometime next week. I have checked the wip/mpc branch, merged master into it, compiled it, and it seems to be working properly, do you consider the MPFR to MPC migration ready on that branch, is there anything else to do?
(In reply to Robert Roth from comment #6) > (In reply to Phillip Wood from comment #5) > > > I've made some progress this week - the tests pass and it seems to work. > > I'll clean things up and push something to wip/mpc sometime next week. > > I have checked the wip/mpc branch, merged master into it, compiled it, and > it seems to be working properly, do you consider the MPFR to MPC migration > ready on that branch, is there anything else to do? Thanks for looking at the branch I'm afraid I got distracted by other things. I think (it’s been a long time since I looked at it) the port to MPC is OK and an improvement on the current implementation. The work to make complex numbers optional is not ready and there could be more tests. I had also hoped to improve the answers from root and power with fraction real powers so that the simplest root (i.e. real or imaginary rather than complex) is returned for things like ∛i which ideally should return -i but currently returns ½√3+½i which is correct but probably not what someone would expect. Realistically I'm not going to get round to that anytime soon. I'll post a patch with the port to MPC for review with a view to getting that part of the branch merged.
Created attachment 336210 [details] [review] Use MPC for complex numbers MPC is a library for complex numbers built on top of MPFR. Using it fixes a number of issues with GNOME Calculators incomplete support for complex numbers and simplifies the implementation of a number of functions. Thanks to Daniel Renninghoff this commit also reworks the MPFR bindings to use vala's automatic memory management. I've added a default rounding mode which makes the calling code cleaner. The use of MPFR.RealRef is not ideal but there doesn't seem to be a better way to deal with references to structs in vala. The lvalue_allowed = false annotation is necessary to make MPC.abs() and MPC.arg() work, otherwise vala stores the result in a copy of the target variable. The bindings for MPC and MPFR.RealRef are licensed GPL3+ as MPFR and MPC are LGPL3 so it doesn't make sense to license them as GPL2+ as the GPL2 is incompatible with LGPL3
Attachment 336210 [details] pushed as 476df14 - Use MPC for complex numbers
Hi, I just wanted to add that after importing commit 476df14 I was getting a failure when building gnome-calculator because of some old generated files still lying around; "make distclean" was not enough to eliminate them. Compilation works fine in a freshly cloned repository, or after eliminating the old files with something like "git clean -f -X". Ciao, Antonio
I'm glad complex numbers are working better for 3.24. What do you think of this minimal patch for older gnome-calculator versions like what is included in Ubuntu 16.04 LTS? https://launchpadlibrarian.net/251890706/complex-exponentiation.patch https://launchpad.net/bugs/1566513
(In reply to Jeremy Bicha from comment #11) > I'm glad complex numbers are working better for 3.24. > > What do you think of this minimal patch for older gnome-calculator versions > like what is included in Ubuntu 16.04 LTS? I could do that, the patch looks minimal indeed. As far as I see, Ubuntu 16.04 Xenial has 3.18, would it be enough for you if I would apply the patch to gnome-3-18, gnome-3-20 and gnome-3-22, and make a release for each?
Oh wow, I wasn't expecting a 3.18 release. I see that Ubuntu 16.04 cherry-picked this commit so you might want to include that if you do make a 3.18 release (it was already included in 3.20.2) https://git.gnome.org/browse/gnome-calculator/commit/?id=647eb687
> Oh wow, I wasn't expecting a 3.18 release. > > I see that Ubuntu 16.04 cherry-picked this commit so you might want to > include that if you do make a 3.18 release (it was already included in > 3.20.2) > https://git.gnome.org/browse/gnome-calculator/commit/?id=647eb687 Well, I offered a 3.18.x release in order to push a fix which can become available for 16.04, as the next LTS, 18.04 is quite far away. :). Ok then, I will apply the patch you have suggested and cherry-pick the other one to gnome-3-18 for you to be able to get rid of the downstream patch, and publish a release sometimes this week. For other issues, feel free to ping me via mail or irc (evfool), let's not spam the other CCs with unrelated patches.