Opened 7 years ago

Closed 7 years ago

#19428 closed enhancement (fixed)

Add support for MPFR_RNDA rounding mode

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-7.0
Component: basic arithmetic Keywords:
Cc: Merged in:
Authors: Jeroen Demeyer Reviewers: Vincent Delecroix, Marc Mezzarobba
Report Upstream: N/A Work issues:
Branch: 9c58daa (Commits, GitHub, GitLab) Commit: 9c58daa5a93b1ffe3b7ba72287453828a4637a5f
Dependencies: Stopgaps:

GitHub link to the corresponding issue

Description (last modified by jdemeyer)

This rounding mode is "new" since MPFR 3.

Also use MPFR 3 names:

  • GMP_RNDx -> MPFR_RNDx
  • mp_prec_t -> mpfr_prec_t
  • mp_rnd_t -> mpfr_rnd_t

Change History (28)

comment:1 Changed 7 years ago by jdemeyer

Branch: → u/jdemeyer/add_support_for_mpfr_rnda_rounding_mode

comment:2 Changed 7 years ago by jdemeyer

Commit: → c8ab56233c3141da1ea37309073384534f866800
Description: modified (diff)
Status: new → needs_review

New commits:

​c8ab562Introduce MPFR_RNDA, rename GMP -> MPFR rounding modes

comment:3 Changed 7 years ago by git

Commit: c8ab56233c3141da1ea37309073384534f866800 → 2a1dd8947de11919a40824b7ccad80fc2331372c

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​2a1dd89Introduce MPFR_RNDA, rename GMP -> MPFR rounding modes

comment:4 Changed 7 years ago by git

Commit: 2a1dd8947de11919a40824b7ccad80fc2331372c → a12282421aff5c5b535643c67ac46a9db5d09f1d

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​a122824Introduce MPFR_RNDA, rename GMP -> MPFR rounding modes

comment:5 Changed 7 years ago by jdemeyer

Description: modified (diff)

comment:6 Changed 7 years ago by vdelecroix

Reviewers: → Vincent Delecroix
Status: needs_review → needs_work

Wrong syntax 

+        TESTS::
+
+        Test the various rounding modes::

comment:7 Changed 7 years ago by git

Commit: a12282421aff5c5b535643c67ac46a9db5d09f1d → 649048e39f7d4d86d89d1dc741173c9b132a0ae4

Branch pushed to git repo; I updated commit sha1. New commits:

​649048eFix documentation formatting

comment:8 Changed 7 years ago by jdemeyer

Status: needs_work → needs_review

comment:9 Changed 7 years ago by git

Commit: 649048e39f7d4d86d89d1dc741173c9b132a0ae4 → 15c471c7bfc2f88355624534c34565015336768a

Branch pushed to git repo; I updated commit sha1. New commits:

​5bf8176Merge tag '7.0.beta2' into t/19428/add_support_for_mpfr_rnda_rounding_mode
​15c471cAlso replace GMP_RNDN -> MPFR_RNDN in interpreters

comment:10 Changed 7 years ago by mmezzarobba

RNDA is marked experimental in the MPFR manual, and Vincent Lefèvre recently insisted on mpfr-devel that it "is not part of the API" and "could disappear entirely without notice", so I'm not sure it is such a good idea to start supporting it unless we have a real use for it.

comment:11 in reply to:  10 ; Changed 7 years ago by jdemeyer

Replying to mmezzarobba:

RNDA is marked experimental in the MPFR manual

I cannot find anything like that in the manual of mpfr-3.1.3 (are you using an outdated version?). What is your reference?

Last edited 7 years ago by jdemeyer (previous) (diff)

comment:12 in reply to:  11 ; Changed 7 years ago by mmezzarobba

Replying to jdemeyer:

Replying to mmezzarobba:

RNDA is marked experimental in the MPFR manual

I cannot find anything like that in the manual of mpfr-3.1.3 (are you using an outdated version?). What is your reference?

First paragraph of Section 4.4 (or just grep for "experimental").

Edit: oh, no, sorry, it was marked as such in the manual of MPFR 3.0, but that no longer seems to be the case with the current release. Well, then I don't know why Vincent said that.

Last edited 7 years ago by mmezzarobba (previous) (diff)

comment:13 in reply to:  12 ; Changed 7 years ago by jdemeyer

Replying to mmezzarobba:

Edit: oh, no, sorry, it was marked as such in the manual of MPFR 3.0

Well, it was a new feature in MPFR 3.0, so it's not surprising that it was experimental initially.

Do you have a link to the post of Vincent Lefèvre?

comment:14 in reply to:  13 Changed 7 years ago by mmezzarobba

Replying to jdemeyer:

Replying to mmezzarobba:

Edit: oh, no, sorry, it was marked as such in the manual of MPFR 3.0

Well, it was a new feature in MPFR 3.0, so it's not surprising that it was experimental initially.

Do you have a link to the post of Vincent Lefèvre?

​https://sympa.inria.fr/sympa/arc/mpfr/2015-12/msg00071.html

I just saw he was talking abound MPFR_RNDNA, which I didn't even know existed. False alarm, sorry.

Last edited 7 years ago by mmezzarobba (previous) (diff)

comment:15 Changed 7 years ago by mmezzarobba

If you're replacing GMP_RNDx by MPFR_RNDx everywhere, I guess you may also want to replace mp_rnd_t by mpfr_rnd_t (and perhaps mp_prec_t by mpfr_prec_t) in mpfr.pxd. Unless there is a particular reason not to do it?

comment:16 in reply to:  15 ; Changed 7 years ago by jdemeyer

Replying to mmezzarobba:

If you're replacing GMP_RNDx by MPFR_RNDx everywhere, I guess you may also want to replace mp_rnd_t by mpfr_rnd_t (and perhaps mp_prec_t by mpfr_prec_t) in mpfr.pxd. Unless there is a particular reason not to do it?

There is no particular reason, I just didn't bother. Will you review it if I make those replacements?

comment:17 in reply to:  16 Changed 7 years ago by mmezzarobba

Replying to jdemeyer:

There is no particular reason, I just didn't bother. Will you review it if I make those replacements?

I don't have much time, but yes, I can try to review it (hopefully next Monday) in any case.

comment:18 Changed 7 years ago by git

Commit: 15c471c7bfc2f88355624534c34565015336768a → 16257d8bf5749e20642f21a2b1b122246e4b8630

Branch pushed to git repo; I updated commit sha1. New commits:

​16257d8Use mpfr_rnd_t and mpfr_prec_t

comment:19 Changed 7 years ago by jdemeyer

Description: modified (diff)

comment:20 Changed 7 years ago by jdemeyer

Milestone: sage-6.10 → sage-7.0
Reviewers: Vincent Delecroix → Vincent Delecroix, Marc Mezzarobba,

comment:21 Changed 7 years ago by jdemeyer

Reviewers: Vincent Delecroix, Marc Mezzarobba, → Vincent Delecroix, Marc Mezzarobba

comment:22 Changed 7 years ago by mmezzarobba

I added two commits with minor improvements (or so I hope!) in u/mmezzarobba/add_support_for_mpfr_rnda_rounding_mode. Please merge that branch if you agree with the changes. Whether you merge them or not, feel free to set the ticket to positive review if all tests pass.

comment:23 Changed 7 years ago by jdemeyer

Branch: u/jdemeyer/add_support_for_mpfr_rnda_rounding_mode → u/mmezzarobba/add_support_for_mpfr_rnda_rounding_mode
Commit: 16257d8bf5749e20642f21a2b1b122246e4b8630 → 9c58daa5a93b1ffe3b7ba72287453828a4637a5f

New commits:

​5d96da0mpmath.ext_impl: simplify rndmode_to_mpfr
​9c58daaSupport RNDA in RealIntervalField._real_field()

comment:24 Changed 7 years ago by jdemeyer

Your changes look good to me. Did you run doctests with your changes?

comment:25 in reply to:  24 Changed 7 years ago by mmezzarobba

Replying to jdemeyer:

Your changes look good to me. Did you run doctests with your changes?

Only the tests in rings/ and libs/.

comment:26 Changed 7 years ago by jdemeyer

To be sure, I will run make ptestlong and set positive review if it passes.

comment:27 Changed 7 years ago by jdemeyer

Status: needs_review → positive_review

comment:28 Changed 7 years ago by vbraun

Branch: u/mmezzarobba/add_support_for_mpfr_rnda_rounding_mode → 9c58daa5a93b1ffe3b7ba72287453828a4637a5f
Resolution: → fixed
Status: positive_review → closed
Note: See TracTickets for help on using tickets.