factorial(algorithm=...) does not work as claimed

[rwst]

It's a defect because the reference/misc/sage/rings/arith.html documentation makes users expect the algorithm keyword to work but

sage: y=factorial(10^6,algorithm='gmp')
---------------------------------------------------------------------------
...
TypeError: __call__() got an unexpected keyword argument 'algorithm'

As the implementation of the algorithm keyword in symbolic function seems difficult (#17531) as pragmatic solution would be to accept that for algorithm to work the arith version of factorial must be explicitly called, see #19461 comment:9 for the general argument.
The rings version apparently gets overwritten on import and the function/ does not accept/transfer the algorithm keyword.

Depends on #24178

CC: @kcrisman @nthiery

Component: symbolics

Keywords: integer symbolic function

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/17489

[rwst]

Description changed:

--- 
+++ 
@@ -8,3 +8,5 @@
 ...
 TypeError: __call__() got an unexpected keyword argument 'algorithm'
 ```
+
+While we're at it, adapt the function's behaviour to what other CASs (sympy, Wolfram) do at integer n<-1: return unsigned infinity.

[rwst]

comment:2

Timing comparison:

• (timeit) 10³!: sympy: 3.38 µs, ginac: 11.6 µs, gmp: 11.8 µs, pari: 22.4 µs
• (time) 10⁶!: sympy: 10s, ginac: 200 ms, gmp: 190 ms, pari: 410 ms
  Here, sympy caches the result (or the computation), so subsequent calls with the same argument are fast. In contrast, the other programs always use 0.2-0.4 seconds per call to factorial(10⁶) so, by wrapping in a @cached function, they would be as fast as sympy in followup calls.

This clearly shows sympy is not up to it, EDIT: and this ticket will speed up many computations 2x.

[rwst]

comment:3

Replying to @rwst:

Timing comparison

In fact, the functions/other version supposedly calling pynac/ginac actually indirectly calls the rings/arith version, i.e., gmp.

So, there will be no speed changes with this ticket.

This, however, means also that the problem with passing algorithm is due to Function_factorial being a GinacFunction. Of course, a GinacFunction would only use Ginac, so it figures.

[rwst]

comment:4

It looks like #9240 abandoned GiNaC behaviour for factorial. Then why keep this as GinacFunction and go through calling rings.arith.factorial via py_factorial_py?

[rwst]

Branch: u/rws/remove_redundant_factorial___from_rings_arith_py

[rwst]

comment:6

Found a way to use algorithm without abandoning the use of GinacFunction.

---

New commits:

4aef058	17489: import factorial only from functions/other
c241edf	17489: move factorial to functions; enable algorithms,hold keywords

[rwst]

Author: Ralf Stephan

[rwst]

Commit: c241edf

[jdemeyer]

comment:7

If you no longer use the py_factorial_py function, it should be deprecated.

[rwst]

comment:8

Replying to @jdemeyer:

If you no longer use the py_factorial_py function, it should be deprecated.

Sure? It is used by the py_factorial doctest.