Grinnz / Math-Calc-Parser

Math::Calc::Parser - Parse and evaluate mathematical expressions
https://metacpan.org/pod/Math::Calc::Parser
Other
0 stars 0 forks source link

t/bigrat.t fails #5

Open eserte opened 2 years ago

eserte commented 2 years ago

On some of my smokers t/bigrat.t fails like this:

#   Failed test 'Evaluated 3/9'
#   at t/bigrat.t line 14.
#          got: '0.3333333333333333333333333333333333333333'
#     expected: '1/3'

#   Failed test 'Evaluated 3>>2'
#   at t/bigrat.t line 16.
#          got: '0.75'
#     expected: '3/4'

#   Failed test 'Division'
#   at t/bigrat.t line 23.
#          got: '1.5'
#     expected: '3/2'

#   Failed test 'Right shift'
#   at t/bigrat.t line 27.
#          got: '1.5'
#     expected: '3/2'
Error in function "round": Can't call Math::BigFloat->as_float, not a valid method
# Tests were run but no plan was declared and done_testing() was not seen.
# Looks like your test exited with 255 just after 20.
t/bigrat.t ............. 
Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 4/20 subtests 
eserte commented 2 years ago

Statistical analysis shows the following possible regressions:

****************************************************************
Regression 'qr:#\s+Math::BigFloat\s+(\S+)'
****************************************************************
Name                   Theta          StdErr     T-stat
[0='const']           1.0000          0.0000    7098027392248595.00
[1='eq_1.9991']       0.0000          0.0000       0.00
[2='eq_1.9997']       0.0000          0.0000       0.00
[3='eq_1.999715']             0.0000          0.0000       0.56
[4='eq_1.999726']            -0.0000          0.0000      -0.64
[5='eq_1.999806']             0.0000          0.0000       0.00
[6='eq_1.999808']            -0.0000          0.0000      -0.64
[7='eq_1.999811']            -0.0000          0.0000      -1.46
[8='eq_1.999816']            -0.0000          0.0000      -1.55
[9='eq_1.999818']             0.0000          0.0000       0.74
[10='eq_1.999829']           -0.0000          0.0000      -1.50
[11='eq_1.999830']           -1.0000          0.0000    -7048563184100897.00

R^2= 1.000, N= 134, K= 12
****************************************************************

****************************************************************
Regression 'qr:#\s+Math::BigRat\s+(\S+)'
****************************************************************
Name                   Theta          StdErr     T-stat
[0='const']           1.0000          0.0000    6017621578413348.00
[1='eq_0.24']        -0.0000          0.0000      -3.07
[2='eq_0.2603']      -0.0000          0.0000      -2.36
[3='eq_0.2604']      -0.0000          0.0000      -2.18
[4='eq_0.2606']      -0.0000          0.0000      -3.55
[5='eq_0.2608']       0.0000          0.0000       1.20
[6='eq_0.260802']            -0.0000          0.0000      -0.29
[7='eq_0.2611']      -0.0000          0.0000      -1.83
[8='eq_0.2613']       0.0000          0.0000       0.64
[9='eq_0.2614']      -0.0000          0.0000      -0.65
[10='eq_0.2619']             -0.0000          0.0000      -2.36
[11='eq_0.2620']             -0.0000          0.0000      -3.80
[12='eq_0.2621']             -1.0000          0.0000    -5761434023392422.00
[13='eq_0.2622']             -1.0000          0.0000    -5968092986545348.00
[14='eq_0.26_02']             0.0000          0.0000       0.00

R^2= 1.000, N= 134, K= 15
****************************************************************
pjacklam commented 2 years ago

The errors go away if you change line 258 in Math::Calc::Parser.pm from

    $token = Math::BigFloat->new($token) if $bignum or $bigrat;

to

    $token = Math::BigFloat->new($token) if $bignum;
    $token = Math::BigRat->new($token)   if $bigrat;

I guess there is a reason behind it, but to me it seems odd to use Math::BigFloat->new() if you want a Math::BigRat. I guess I am missing something.

Grinnz commented 2 years ago

That is after parsing a token that looks like a number, which would not be appropriate to initialize a BigRat. BigFloat is configured to upgrade to BigRat.

pjacklam commented 2 years ago

I'm afraid I don't understand. The current code is

    ...
    } elsif (Scalar::Util::looks_like_number $token) {
            $token = Math::BigFloat->new($token) if $bignum or $bigrat;
            _shunt_number(\@expr_queue, \@oper_stack, $token);
            $binop_possible = 1;
    } elsif ($token =~ m/\A\w+\z/) {
    ...

To me it looks like Math::BigFloat->new() is called when (not after) trying to parse a token that looks like a number. Out of curiosity, I printed out the complete list of tokens that are passed to Math::BigFloat->new() when the file t/bigrat.t is executed, and the tokens look very much like numbers. Here is the complete list: 3, 9, 3, 2, 3, 2, 3, 2, 3, 2, 3, 2, 3, 2, 3, 2, 3, 2, 3, 1, 3, 2, 2, 5, 2, 5, 2, 5, 2, 5, 2, 5, 2, 5, 2, 5, 2, 5, 2, 1, 64. What am I missing?

Grinnz commented 2 years ago

Yes, the token will always be a Perlish number. That could be an integer or float and will never look like '3/5'.

pjacklam commented 2 years ago

Yes, the token will always be a Perlish number. That could be an integer or float and will never look like '3/5'.

I see. Thanks for the explanation. But I wonder. Why are tokens converted to Math::BigFloat objects also when emulating bigrat?

Grinnz commented 2 years ago

BigFloat is the only class that would be appropriate to initialize from any of the possible numeric tokens. Even in bigrat mode the expression can contain floats. Upgrading should result in BigRats where appropriate.

Grinnz commented 2 years ago

But it's entirely possible there's a better way to achieve this, I don't have a good understanding of the upgrading/downgrading functionality of Math::Big*.

pjacklam commented 2 years ago

BigFloat is the only class that would be appropriate to initialize from any of the possible numeric tokens. Even in bigrat mode the expression can contain floats. Upgrading should result in BigRats where appropriate.

Any numeric token that is accepted by Math::BigFloat should also be accepted by Math::BigRat, and the resulting Math::BigRat should represent the exact same value as the Math::BigFloat. Anything else is a bug. Here is an example where the input is an approximate hexadecimal floating point representation of pi.

use v5.14;
use Math::BigFloat;
use Math::BigRat;

# pi as hex float string
my $pi_str = sprintf "%a", atan2 0, -1;
say $pi_str;     # "0x1.921fb54442d18p+1"

# convert the hex string to a Math::BigRat
my $pi_mbr = Math::BigRat -> new($pi_str);
say $pi_mbr;     # 884279719003555/281474976710656

# convert the hex string to a Math::BigFloat
my $pi_mbf = Math::BigFloat -> new($pi_str);
say $pi_mbf;    # 3.141592653589793115997963468544185161590576171875

# convert the rational number to a Math::BigFloat by division
my $pi_div = Math::BigFloat -> new("884279719003555")
                            -> bdiv("281474976710656", 49);
say $pi_div;    # 3.141592653589793115997963468544185161590576171875

In the division, I had to use an accuracy of 49 because the default accuracy of 40 is not sufficient to get the exact result of the division. Anyway, if there is any numeric token that is not handled correctly by Math::BigRat, I would like to know about it.

As for the upgrading and downgrading, I have not been able to find any documentation stating how it is supposed to work, so I am not sure about this either. Looking at the code, it seems that if the result of a computation can't be represented in the current class, then upgrade, if upgrading is enabled. It makes sense for Math::BigFloat to upgrade a division like 1/3 from Math::BigFloat to Math::BigRat, but on the other hand, upgrading log(2) to Math::BigRat doesn't seem very helpful, especially since Math::BigRat doesn't support rounding.

In version 1.999836 of the Math-BigInt distribution, I have re-enabled upgrading in bdiv(), so there should be no more failed tests in t/bigrat.t.

Grinnz commented 2 years ago

Thanks for the explanation. It would make sense to create a Math::BigRat in that case.