After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 759439 - Use MPC instead of MPFR
Use MPC instead of MPFR
Status: RESOLVED FIXED
Product: gnome-calculator
Classification: Core
Component: general
3.18.x
Other Linux
: Normal normal
: ---
Assigned To: gcalctool maintainers
gcalctool maintainers
Depends on:
Blocks:
 
 
Reported: 2015-12-14 11:19 UTC by Phillip Wood
Modified: 2017-02-06 18:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch for automatic memory management (19.32 KB, patch)
2015-12-15 00:26 UTC, Daniel Renninghoff
none Details | Review
Use MPC for complex numbers (51.54 KB, patch)
2016-09-25 10:11 UTC, Phillip Wood
committed Details | Review

Description Phillip Wood 2015-12-14 11:19:48 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
Comment 1 Phillip Wood 2015-12-14 11:24:25 UTC
>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.
Comment 2 Daniel Renninghoff 2015-12-15 00:26:34 UTC
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.
Comment 3 Alberto Ruiz 2016-01-11 21:24:49 UTC
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.
Comment 4 Phillip Wood 2016-01-15 19:22:24 UTC
(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.
Comment 5 Phillip Wood 2016-01-15 19:28:04 UTC
(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.
Comment 6 Robert Roth 2016-09-25 02:03:16 UTC
(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?
Comment 7 Phillip Wood 2016-09-25 10:04:34 UTC
(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.
Comment 8 Phillip Wood 2016-09-25 10:11:08 UTC
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
Comment 9 Robert Roth 2016-10-11 07:58:13 UTC
Attachment 336210 [details] pushed as 476df14 - Use MPC for complex numbers
Comment 10 Antonio Ospite 2016-10-11 11:43:49 UTC
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
Comment 11 Jeremy Bicha 2017-02-06 07:01:33 UTC
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
Comment 12 Robert Roth 2017-02-06 18:16:04 UTC
(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?
Comment 13 Jeremy Bicha 2017-02-06 18:32:53 UTC
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
Comment 14 Robert Roth 2017-02-06 18:59:27 UTC
> 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.