Limit loop to compute factorial to n<=170, for n>170 return Perl Inf#682
Limit loop to compute factorial to n<=170, for n>170 return Perl Inf#682pstaabp merged 2 commits intoopenwebwork:PG-2.17from
Conversation
as that would be the result of the calculation used for any such n due to overflow of the Perl floating point data type.
lib/Parser/UOP/factorial.pm
Outdated
| my $n = shift; my $f = 1; | ||
| $self->Error("Factorial can only be taken of (non-negative) integers") | ||
| unless $n =~ m/^\d+$/; | ||
| return Inf if $n > 170; |
There was a problem hiding this comment.
I'm wondering if this should produce a MathObject Infinity rather than a Perl Inf. (My comment on the original discussion used Inf to make it work like the other ones in Alex's tests.)
I get error messages about Inf being a bareword for this line. I think it may need to be
| return Inf if $n > 170; | |
| return $self->Package("Infinity")->new() if $n > 170; |
There was a problem hiding this comment.
I tried the proposed change now.
Even with that change putting 171! inside a n input box for a Formula results in the error message Can't convert an Infinity to a constant.
Compute( "171!" ); also triggers that error for either option of the return value for n>170 (and also when the prior version of the code is used).
As such, it seems to me that keep the return value as a Perl infinity (which it would have been in the past) is a more certain way of maintaining backwards compatibility, and saves on any overhead of creating an object here when it has no purpose in the end.
I do see a warning about the bareword from perl -C lib/Parser/UOP/factorial.pm but it does not seem to interfere when used inside pg.
However, if you still think the change is advisable, I will make it.
There was a problem hiding this comment.
Even with that change putting 171! inside a n input box for a Formula results in the error message Can't convert an Infinity to a constant.
I am not able to reproduce that. It works correctly for me (reporting an entered value of infinity and that the value is incorrect. No error message is produced. The code that converts to a constant does handle Infinity MathObjects (but not perl Inf, though perhaps it should).
There was a problem hiding this comment.
I upgraded my version of PG. The Docker container had a somewhat old version. Now it works properly. Sorry.
There was a problem hiding this comment.
Note: Peter's PR had the recommended change in it.
There was a problem hiding this comment.
That is now merged, so $self->Package("Infinity")->new() is not the return value of n>170.
|
This also seems like we should have a unit-test for this. I wrote a number of unit tests last summer and hoped to do more. Let me see if I can pull one together to add to this PR. |
|
I just wrote a factorial unit test and added as a PR to your branch @taniwallach. |
* Added a test for the factorial. * FIX: clarified some of the language in the tests.
4 participants
Fixes the issue from #675 by returning Perl
Inffor any integer n>170, and using the current approach for integer values of n in [0,170]. That seems to be the consensus on the best patch for the problem itself. Non-integervalues will return the same error message as before.