Opened 3 years ago
Closed 2 years ago
#26749 closed defect (fixed)
Let `factorial` accept float arguments
Reported by:  pelegm  Owned by:  

Priority:  major  Milestone:  sage9.1 
Component:  symbolics  Keywords:  plot, factorial, gamma, float 
Cc:  mmezzarobba, slelievre, tmonteil, vdelecroix  Merged in:  
Authors:  Vincent Delecroix  Reviewers:  Samuel Lelièvre 
Report Upstream:  N/A  Work issues:  
Branch:  77389bc (Commits, GitHub, GitLab)  Commit:  77389bcc2ce2f10e61a0a1ba165c4f9316a5bbb8 
Dependencies:  Stopgaps: 
Description (last modified by )
In Sage 8.3 to 9.0, factorial
chokes if fed a float argument:
sage: factorial(float(3.2)) Traceback (most recent call last): ... AttributeError: 'float' object has no attribute 'gamma'
We change the numerical evaluation so it will accept a float argument.
With this ticket:
sage: factorial(float(3.2)) 7.756689535793181
As a consequence, factorial(x)
can be plotted again  as gamma(x + 1).
Change History (21)
comment:1 followup: ↓ 3 Changed 3 years ago by
 Branch set to u/ghdurgeshra/cannot_plot_factorial
comment:2 Changed 3 years ago by
 Commit set to bc795d609f8b403df23fbe8a2c8b433292a9222f
comment:3 in reply to: ↑ 1 Changed 3 years ago by
 Status changed from new to needs_review
Replying to ghdurgeshra:
Example:
sage: factorial(x).plot()  ArithmeticError Traceback (most recent call last) <ipythoninput1d07fdfec6413> in <module>() > 1 factorial(x).plot() /home/durgesh/Downloads/sage8.4/local/lib/python2.7/sitepackages/sage/symbolic/expression.pyx in sage.symbolic.expression.Expression.plot (build/cythonized/sage/symbolic/expression.cpp:63513)() > 12067 except NotImplementedError: /home/durgesh/Downloads/sage8.4/local/lib/python2.7/sitepackages/sage/misc/decorators.pyc in wrapper(*args, **kwds) > 567 return func(*args, **options) /home/durgesh/Downloads/sage8.4/local/lib/python2.7/sitepackages/sage/plot/plot.py in plot(funcs, *args, **kwds) > 1956 G = _plot(funcs, (xmin, xmax), **kwds) /home/durgesh/Downloads/sage8.4/local/lib/python2.7/sitepackages/sage/plot/plot.py in _plot(funcs, xrange, parametric, polar, fill, label, randomize, **options) > 2266 randomize) /home/durgesh/Downloads/sage8.4/local/lib/python2.7/sitepackages/sage/plot/plot.py in generate_plot_points(f, xrange, plot_points, adaptive_tolerance, adaptive_recursion, randomize, initial_points) > 3731 raise ArithmeticError("The function cannot be plotted.") ArithmeticError: The function cannot be plotted.
(raises arithmetic error)
sage: (x).plot() Launched png viewer for Graphics object consisting of 1 graphics primitive
(plots correctly)
comment:4 followup: ↓ 5 Changed 3 years ago by
Don't mask errors. By putting a whole statement in a bare "try/except" you make it very hard to debug the code, because *any* error that gets raised will now be masked by "ArithmeticError?".
Note that gamma(x+1).plot()
works just fine, so what factorial(x).plot()
tries to do could just be fine. If you absolutely think this example needs to perform better then the way that factorial
tries to evaluate numerical arguments should probably be adjusted.
Here you can see that the original error message is much more informative than what you suggest on this ticket: it actually tells what is going wrong and allows someone to come up with a fix. Carpeting over this very useful error message with a generic, uninformative function cannot be plotted
is not an improvement at all.
comment:5 in reply to: ↑ 4 Changed 3 years ago by
Replying to nbruin:
Don't mask errors. By putting a whole statement in a bare "try/except" you make it very hard to debug the code, because *any* error that gets raised will now be masked by "ArithmeticError?".
Note that
gamma(x+1).plot()
works just fine, so whatfactorial(x).plot()
tries to do could just be fine. If you absolutely think this example needs to perform better then the way thatfactorial
tries to evaluate numerical arguments should probably be adjusted.Here you can see that the original error message is much more informative than what you suggest on this ticket: it actually tells what is going wrong and allows someone to come up with a fix. Carpeting over this very useful error message with a generic, uninformative
function cannot be plotted
is not an improvement at all.
So what should factorial(x).plot()
return? Should it make a plot like the gamma function?
comment:6 Changed 2 years ago by
 Cc mmezzarobba slelievre tmonteil vdelecroix added
 Keywords changed from plot,factorial,gamma,float to plot, factorial, gamma, float
 Milestone changed from sage8.5 to sage9.1
The factorial
function used to accept a Python float
as an argument, but no longer does. It might have to do
with the refactoring in #25421. Compare the following.
In Sage 8.2:
sage: version() 'SageMath version 8.2, Release Date: 20180505' sage: factorial(1.2) 1.10180249087971 sage: factorial(1.2r) 1.1018024908797128
In Sage 8.3:
sage: version() 'SageMath version 8.3, Release Date: 20180803' sage: factorial(1.2) 1.10180249087971 sage: factorial(1.2r) Traceback (most recent call last) ... AttributeError: 'float' object has no attribute 'gamma'
The problem manifests itself when plotting because when plotting a symbolic function Sage feeds it Python floats.
comment:7 Changed 2 years ago by
 Branch changed from u/ghdurgeshra/cannot_plot_factorial to public/26749
 Commit changed from bc795d609f8b403df23fbe8a2c8b433292a9222f to 684d323097a51706709430dceea72114b23a956c
 Description modified (diff)
 Summary changed from Cannot plot factorial to Factorial can not handle float argument
New commits:
684d323  26749: make factorial function accepts float argument

comment:8 Changed 2 years ago by
I like this try/except solution! Positive review from me if the patchbots are happy.
comment:9 Changed 2 years ago by
 Description modified (diff)
 Summary changed from Factorial can not handle float argument to Let `factorial` accept float arguments
comment:10 Changed 2 years ago by
 Description modified (diff)
 Reviewers set to Samuel Lelièvre
comment:11 followup: ↓ 12 Changed 2 years ago by
Note: a similar problem for sqrt
was reported at
comment:12 in reply to: ↑ 11 ; followup: ↓ 13 Changed 2 years ago by
Replying to slelievre:
Note: a similar problem for
sqrt
was reported at
What do you mean exactly? The following is with 9.1.beta4
sage: sqrt(float(1.3)) 1.140175425099138 sage: type(sqrt(float(1.3))) <class 'float'>
comment:13 in reply to: ↑ 12 Changed 2 years ago by
Replying to vdelecroix:
Replying to slelievre:
Note: a similar problem for
sqrt
was reported atWhat do you mean exactly? The following is with 9.1.beta4
sage: sqrt(float(1.3)) 1.140175425099138 sage: type(sqrt(float(1.3))) <class 'float'>
This was a call to .sqrt()
that floats do not have.
comment:14 Changed 2 years ago by
See #29232 for that other problem, sorry for the noise here.
comment:15 Changed 2 years ago by
 Status changed from needs_review to positive_review
This looks good to me.
comment:16 Changed 2 years ago by
 Status changed from positive_review to needs_work
On 32bit
********************************************************************** File "src/sage/functions/other.py", line 1471, in sage.functions.other.Function_factorial._eval_ Failed example: factorial(float(3.2)) Expected: 7.756689535793181 Got: 7.756689535793178 ********************************************************************** 1 item had failures: 1 of 11 in sage.functions.other.Function_factorial._eval_ [448 tests, 1 failure, 26.12 s]  sage t long src/sage/functions/other.py # 1 doctest failed 
comment:17 Changed 2 years ago by
Turns out that 32 bits is much more accurate!
sage: RealBallField(64)("4.2").gamma() [7.7566895357931776 +/ 5.95e17]
comment:18 Changed 2 years ago by
 Commit changed from 684d323097a51706709430dceea72114b23a956c to 77389bcc2ce2f10e61a0a1ba165c4f9316a5bbb8
comment:19 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:20 Changed 2 years ago by
 Status changed from needs_review to positive_review
Thanks! Positive review.
comment:21 Changed 2 years ago by
 Branch changed from public/26749 to 77389bcc2ce2f10e61a0a1ba165c4f9316a5bbb8
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Change tabs to spaces