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: sage-9.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:

Status badges

Description (last modified by slelievre)

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 follow-up: Changed 3 years ago by gh-durgeshra

  • Branch set to u/gh-durgeshra/cannot_plot_factorial

comment:2 Changed 3 years ago by git

  • Commit set to bc795d609f8b403df23fbe8a2c8b433292a9222f

Branch pushed to git repo; I updated commit sha1. New commits:

bc795d6Change tabs to spaces

comment:3 in reply to: ↑ 1 Changed 3 years ago by gh-durgeshra

  • Status changed from new to needs_review

Replying to gh-durgeshra:

Example:

sage: factorial(x).plot()
---------------------------------------------------------------------------
ArithmeticError                           Traceback (most recent call last)
<ipython-input-1-d07fdfec6413> in <module>()
----> 1 factorial(x).plot()

/home/durgesh/Downloads/sage-8.4/local/lib/python2.7/site-packages/sage/symbolic/expression.pyx in sage.symbolic.expression.Expression.plot (build/cythonized/sage/symbolic/expression.cpp:63513)()
> 12067             except NotImplementedError:

/home/durgesh/Downloads/sage-8.4/local/lib/python2.7/site-packages/sage/misc/decorators.pyc in wrapper(*args, **kwds)
--> 567             return func(*args, **options)

/home/durgesh/Downloads/sage-8.4/local/lib/python2.7/site-packages/sage/plot/plot.py in plot(funcs, *args, **kwds)
-> 1956             G = _plot(funcs, (xmin, xmax), **kwds)

/home/durgesh/Downloads/sage-8.4/local/lib/python2.7/site-packages/sage/plot/plot.py in _plot(funcs, xrange, parametric, polar, fill, label, randomize, **options)
-> 2266                                     randomize)

/home/durgesh/Downloads/sage-8.4/local/lib/python2.7/site-packages/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 follow-up: Changed 3 years ago by 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 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 gh-durgeshra

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 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.

So what should factorial(x).plot() return? Should it make a plot like the gamma function?

comment:6 Changed 2 years ago by slelievre

  • Cc mmezzarobba slelievre tmonteil vdelecroix added
  • Keywords changed from plot,factorial,gamma,float to plot, factorial, gamma, float
  • Milestone changed from sage-8.5 to sage-9.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: 2018-05-05'
sage: factorial(1.2)
1.10180249087971
sage: factorial(1.2r)
1.1018024908797128

In Sage 8.3:

sage: version()
'SageMath version 8.3, Release Date: 2018-08-03'
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 vdelecroix

  • Branch changed from u/gh-durgeshra/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:

684d32326749: make factorial function accepts float argument

comment:8 Changed 2 years ago by slelievre

I like this try/except solution! Positive review from me if the patchbots are happy.

comment:9 Changed 2 years ago by slelievre

  • 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 slelievre

  • Authors set to Vincent Delecroix
  • Description modified (diff)
  • Reviewers set to Samuel Lelièvre

comment:11 follow-up: Changed 2 years ago by slelievre

Note: a similar problem for sqrt was reported at

comment:12 in reply to: ↑ 11 ; follow-up: Changed 2 years ago by vdelecroix

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 chapoton

Replying to vdelecroix:

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'>

This was a call to .sqrt() that floats do not have.

comment:14 Changed 2 years ago by slelievre

See #29232 for that other problem, sorry for the noise here.

comment:15 Changed 2 years ago by was

  • Status changed from needs_review to positive_review

This looks good to me.

comment:16 Changed 2 years ago by vbraun

  • Status changed from positive_review to needs_work

On 32-bit

**********************************************************************
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 vdelecroix

Turns out that 32 bits is much more accurate!

sage: RealBallField(64)("4.2").gamma()                                                                    
[7.7566895357931776 +/- 5.95e-17]

comment:18 Changed 2 years ago by git

  • Commit changed from 684d323097a51706709430dceea72114b23a956c to 77389bcc2ce2f10e61a0a1ba165c4f9316a5bbb8

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

8da470c26749: make factorial function accepts float argument
77389bc26749: add fp tolerance

comment:19 Changed 2 years ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:20 Changed 2 years ago by slelievre

  • Status changed from needs_review to positive_review

Thanks! Positive review.

comment:21 Changed 2 years ago by vbraun

  • Branch changed from public/26749 to 77389bcc2ce2f10e61a0a1ba165c4f9316a5bbb8
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.