Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.91k stars 542 forks source link

Math::BigInt->new() wrongly converts large floats #13569

Open p5pRT opened 10 years ago

p5pRT commented 10 years ago

Migrated from rt.perl.org#121136 (status was 'open')

Searchable as RT121136$

p5pRT commented 10 years ago

From perlmathbigint@volkerschatz.com

  Hello\,

a conversion of large integral floating-point numbers into BigInts can lose significant digits. Test case​: 504403158265495552.0 == 7 * 2**56\, which is exactly representable as a float/double.

  $ perl -MMath​::BigInt -e 'print Math​::BigInt->new(504403158265495552.0)->bstr()\, "\n";' 504403158265496000

The cause seems to be that the _split function in Math​::BigInt treats its argument as a string and thereby triggers Perl's automatic conversion which does not output enough digits for large integers. A workaround is to explicitly convert via sprintf("%f"\,..)\, which retains all digits before the decimal point. A precision could be added (sprintf("%.${P}f"\,..)) if a sub-integer Precision value is in force. (Aside​: such a P value would be negative according to the man page text\, but positive according to the example table - one of them needs correcting.) I am less sure how to handle an Accuracy value or how to prevent unneeded conversions.

My Perl version is 5.18.2\, the Math​::BigInt version 1.9991.

Best regards\,

Volker

p5pRT commented 10 years ago

From dana@acm.org

On Sat Feb 01 06​:35​:27 2014\, perlmathbigint@​volkerschatz.com wrote​:

a conversion of large integral floating-point numbers into BigInts can lose significant digits. Test case​: 504403158265495552.0 == 7 * 2**56\, which is exactly representable as a float/double.

$ perl -MMath​::BigInt -e 'print Math​::BigInt-

new(504403158265495552.0)->bstr()\, "\n";' 504403158265496000

[...]

This ought to go on the Math​::BigInt RT list. Also see RT 61887 (still open) which shows inconsistent behavior with floats that have non-zero fractional parts\, which means more changes are necessary in this code.

This may fall under the "don't do this" clause in the description. Use int($n) to work around. Quoting the value also works\, though is a bit awkward if you have a double to begin with\, forcing you to do Math​::BigInt->new(sprintf("%f"\,$n)).

It would be my belief that the conversion through %f would be a bad idea in general\, as it would lose precision for strings and large integers. If we had something like Scalar​::Number we could convert to a full precision string if and only if the input was an NV.

A hack for testing (don't do this)\, is to insert right before the _split in new​:   use Scalar​::Util 'isdual';   $wanted = int($wanted) if isdual($wanted) && $wanted == int($wanted);

Dana

p5pRT commented 10 years ago

The RT System itself - Status changed from 'new' to 'open'

p5pRT commented 10 years ago

From perlmathbigint@volkerschatz.com

  Hi Dana\,

thank you for your reply.

This ought to go on the Math​::BigInt RT list.

Sorry for that... I was not sure where to send the report\, and in the past for other modules that come with Perl\, I had been told to mail to perlbug.

Use int($n) to work around.

This works only when int() can convert to a 64-bit fixed point type\, and the whole point of BigInt is to allow larger numbers than that. Consider 7 * 2**66 = 516508834063867445248.0 \, which shows the same problem.

It would be my belief that the conversion through %f would be a bad idea in general\, as it would lose precision for strings and large integers.

Yes\, I agree that it would likely mess up non-floats.

If we had something like Scalar​::Number we could convert to a full precision string if and only if the input was an NV.

Thanks for mentioning Scalar​::Number and Scalar​::Util\, which I had not been aware of. I agree about your suggestion. Are you saying "if we had..." because Math​::BigInt as a core Perl module cannot depend on Scalar​::Number?

Anyway\, I think the following would work too​:

@​@​ -3048\,6 +3050\,8 @​@​   # invalid input.   my $x = shift;

+ $x= sprintf("%f"\, $x) unless "$x" == $x; # ensure enough digits from float +   # strip white space at front\, also extraneous leading zeros   $x =~ s/^\s*([-]?)0*([0-9])/$1$2/g; # will not strip ' .2'   $x =~ s/^\s+//; # but this will

The condition checks the integrity of a roundtrip conversion via the automatic stringification and uses sprintf if it fails.

I have taken a look at some other bugs\, and #63038 is fixed by using substr() to strip digits instead of a regex​:

@​@​ -625\,8 +625\,10 @​@​   {   # xE-y\, and empty mfv   #print "xE-y\n"; - $e = abs($e); - if ($$miv !~ s/0{$e}$//) # can strip so many zero's? + my $tail= substr($$miv\, $e); # abs($e) trailing digits + substr($$miv\, $e)= ""; + $$miv= "0" unless CORE​::length $$miv; + if ($tail =~ /[^0]/)   {   if ($_trap_nan)

Regarding #61887​: The code (lines 613 to 623) shows that the intention was to return NaN for non-integers. Line 641​:

$self->{sign} = '+' if $$miv eq '0';

defeats this when the integer part is 0. To keep the old behaviour\, one would have to move or copy this line elsewhere. To always do a truncation as suggested in the discussion of #61887\, lines 613 to 623 could simply be erased. But that would be inconsistent with creating a NaN for negative exponents stripping non-zero digits\, so the if ($tail =~ /[^0]/) { ... } we have just corrected for #63038 should probably be removed too.

Considering your remark about posting on the Math​::BigInt bug tracker\, I am unsure where to go from here. Is Peter John Acklam still the maintainer\, or are you? Should I submit the patches separately to their respective bug report threads (after creating one for my own issue)? Or should I submit one big patch\, because the ones for #63038 and #61887 will probably overlap?

Regards\,

Volker