Opened 7 years ago

Closed 6 years ago

#14448 closed defect (fixed)

Bad sign in sign_mantissa_exponent() methods

Reported by: tmonteil Owned by: AlexGhitza
Priority: major Milestone: sage-5.10
Component: basic arithmetic Keywords: mpfr, RDF
Cc: jason, zimmerma Merged in: sage-5.10.beta0
Authors: Thierry Monteil Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by tscrim)

The .sign_mantissa_exponent() methods of the RealNumber and RealDoubleElement classes give a negative mantissa when the number is negative, which does not corresponds to the behaviour described in the documentation : http://www.sagemath.org/doc/reference/rings_numerical/sage/rings/real_mpfr.html http://www.sagemath.org/doc/reference/rings_numerical/sage/rings/real_double.html

We propose here to fix it.

By the way, the variable name 'mantissa' sometimes (always?) appears in the source with the meaning of a signed mantissa. I would suggest to renamed it signed_mantissa or s_mantissa to ease code reading (though i do not feel able to detect which one is signed or not along the code). See for example the .exact_rational() method in real_mpfr.pyx.


Apply: trac_14448_sign_of_mantissa-tm.2.patch

Attachments (2)

trac_14448_sign_of_mantissa-tm.patch (2.7 KB) - added by tmonteil 7 years ago.
Tested on sage 5.9.beta5
trac_14448_sign_of_mantissa-tm.2.patch (2.8 KB) - added by tmonteil 7 years ago.
Patch updated so that it quotes this trac ticket

Download all attachments as: .zip

Change History (9)

Changed 7 years ago by tmonteil

Tested on sage 5.9.beta5

comment:1 Changed 7 years ago by tmonteil

  • Cc jason added
  • Description modified (diff)
  • Keywords RDF added; float removed

comment:2 Changed 7 years ago by tscrim

I'm guessing this is ready for review?

Looks good, but one small thing, could you change line 2930 in real_mpfr.pyx to:

The mantissa is always a nonnegative number (see :trac:`14448`)::

or something to this extent, noting the problem was fixed in this ticket?

Thanks,
Travis

comment:3 Changed 7 years ago by jdemeyer

I think it's a bug that the mantissa of 1 and -1 are different.

comment:4 Changed 7 years ago by zimmerma

  • Cc zimmerma added

Changed 7 years ago by tmonteil

Patch updated so that it quotes this trac ticket

comment:5 Changed 7 years ago by tmonteil

  • Status changed from new to needs_review

I added the reference to this trac ticket. I also put this ticket under review.

comment:6 Changed 6 years ago by tscrim

  • Authors set to Thierry Monteil
  • Description modified (diff)
  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Looks good to me.

comment:7 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.10.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.