Opened 7 years ago
Closed 7 years ago
#19428 closed enhancement (fixed)
Add support for MPFR_RNDA rounding mode
Reported by:  Jeroen Demeyer  Owned by:  

Priority:  major  Milestone:  sage7.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: 
Description (last modified by )
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
Branch:  → u/jdemeyer/add_support_for_mpfr_rnda_rounding_mode 

comment:2 Changed 7 years ago by
Commit:  → c8ab56233c3141da1ea37309073384534f866800 

Description:  modified (diff) 
Status:  new → needs_review 
comment:3 Changed 7 years ago by
Commit:  c8ab56233c3141da1ea37309073384534f866800 → 2a1dd8947de11919a40824b7ccad80fc2331372c 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
2a1dd89  Introduce MPFR_RNDA, rename GMP > MPFR rounding modes

comment:4 Changed 7 years ago by
Commit:  2a1dd8947de11919a40824b7ccad80fc2331372c → a12282421aff5c5b535643c67ac46a9db5d09f1d 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
a122824  Introduce MPFR_RNDA, rename GMP > MPFR rounding modes

comment:5 Changed 7 years ago by
Description:  modified (diff) 

comment:6 Changed 7 years ago by
Reviewers:  → Vincent Delecroix 

Status:  needs_review → needs_work 
Wrong syntax
+ TESTS:: + + Test the various rounding modes::
comment:7 Changed 7 years ago by
Commit:  a12282421aff5c5b535643c67ac46a9db5d09f1d → 649048e39f7d4d86d89d1dc741173c9b132a0ae4 

Branch pushed to git repo; I updated commit sha1. New commits:
649048e  Fix documentation formatting

comment:8 Changed 7 years ago by
Status:  needs_work → needs_review 

comment:9 Changed 7 years ago by
Commit:  649048e39f7d4d86d89d1dc741173c9b132a0ae4 → 15c471c7bfc2f88355624534c34565015336768a 

comment:10 followup: 11 Changed 7 years ago by
RNDA is marked experimental in the MPFR manual, and Vincent Lefèvre recently insisted on mpfrdevel 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 followup: 12 Changed 7 years ago by
Replying to mmezzarobba:
RNDA is marked experimental in the MPFR manual
I cannot find anything like that in the manual of mpfr3.1.3 (are you using an outdated version?). What is your reference?
comment:12 followup: 13 Changed 7 years ago by
Replying to jdemeyer:
Replying to mmezzarobba:
RNDA is marked experimental in the MPFR manual
I cannot find anything like that in the manual of mpfr3.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.
comment:13 followup: 14 Changed 7 years ago by
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 Changed 7 years ago by
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/201512/msg00071.html
I haven't read the thread in detail, perhaps the context actually makes it clear that he's talking about a particular use of MPFR_RNDA only.
comment:15 followup: 16 Changed 7 years ago by
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 followup: 17 Changed 7 years ago by
Replying to mmezzarobba:
If you're replacing
GMP_RNDx
byMPFR_RNDx
everywhere, I guess you may also want to replacemp_rnd_t
bympfr_rnd_t
(and perhapsmp_prec_t
bympfr_prec_t
) inmpfr.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 Changed 7 years ago by
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
Commit:  15c471c7bfc2f88355624534c34565015336768a → 16257d8bf5749e20642f21a2b1b122246e4b8630 

Branch pushed to git repo; I updated commit sha1. New commits:
16257d8  Use mpfr_rnd_t and mpfr_prec_t

comment:19 Changed 7 years ago by
Description:  modified (diff) 

comment:20 Changed 7 years ago by
Milestone:  sage6.10 → sage7.0 

Reviewers:  Vincent Delecroix → Vincent Delecroix, Marc Mezzarobba, 
comment:21 Changed 7 years ago by
Reviewers:  Vincent Delecroix, Marc Mezzarobba, → Vincent Delecroix, Marc Mezzarobba 

comment:22 Changed 7 years ago by
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
Branch:  u/jdemeyer/add_support_for_mpfr_rnda_rounding_mode → u/mmezzarobba/add_support_for_mpfr_rnda_rounding_mode 

Commit:  16257d8bf5749e20642f21a2b1b122246e4b8630 → 9c58daa5a93b1ffe3b7ba72287453828a4637a5f 
comment:24 followup: 25 Changed 7 years ago by
Your changes look good to me. Did you run doctests with your changes?
comment:25 Changed 7 years ago by
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
To be sure, I will run make ptestlong
and set positive review if it passes.
comment:27 Changed 7 years ago by
Status:  needs_review → positive_review 

comment:28 Changed 7 years ago by
Branch:  u/mmezzarobba/add_support_for_mpfr_rnda_rounding_mode → 9c58daa5a93b1ffe3b7ba72287453828a4637a5f 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
Introduce MPFR_RNDA, rename GMP > MPFR rounding modes