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]

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.

[jdemeyer]

comment:9

If it's only used by doctests, it's not used :-)

[rwst]

comment:10

Replying to @jdemeyer:

If it's only used by doctests, it's not used :-)

And remove the doctest?

In the same file there are more such wrappers: doublefactorial,test_binomial,py_real_for_doctests etc.

[jdemeyer]

comment:11

Replying to @rwst:

Replying to @jdemeyer:

If it's only used by doctests, it's not used :-)

And remove the doctest?

No, add the deprecation message in the doctest.

[jdemeyer]

comment:12

Also, instead of removing the factorial function from rings/arith.py, it should be deprecated (or perhaps a lazy import with deprecation).

[kcrisman]

Reviewer: Jeroen Demeyer

[sagetrac-git]

Changed commit from c241edf to 3b54c75

[sagetrac-git]

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

80274a1	Merge branch 'develop' into t/17489/remove_redundant_factorial___from_rings_arith_py
3b54c75	17489: deprecate superfluous functions

[rwst]

comment:15

Replying to @jdemeyer:

Replying to @rwst:

Replying to @jdemeyer:
And remove the doctest?

No, add the deprecation message in the doctest.

Not possible because only the first message is printed. I removed the doctests.

Also, instead of removing the factorial function from rings/arith.py, it should be deprecated (or perhaps a lazy import with deprecation).

Any import of factorial is overwritten by sage.functions.other.factorial

[jdemeyer]

comment:16

This is overly complicated:

    try:
        x_in_ZZ = ZZ(x)
        coercion_success = True
    except TypeError:
        coercion_success = False

    if coercion_success and x_in_ZZ >= 0:
        return ZZ(x).factorial()
    else:
        return py_tgamma(x+1)

Why not

    try:
        x = ZZ(x)
    except TypeError:
        pass
    else:
        if x >= 0:
            return x.factorial()

    return py_tgamma(x+1)

Also: the :trac:`9240` syntax should be used in docstrings, not comments in code, so please revert that.

[rwst]

Branch: u/rws/factorial_algorithm______does_not_work_as_claimed

[rwst]

Author: Ralf Stephan

[rwst]

comment:47

Jeroen, are you still reviewer of this ticket?

---

New commits:

b27a3c4

17489: amend documentation

[rwst]

Commit: b27a3c4

[rwst]

Changed commit from b27a3c4 to none

[rwst]

comment:48

I think we are coming to the conclusion that functions with additional keywords (other than hold) need to be implemented as global interface function + symbolic specialization pair. This split has proven beneficial to #24554 as well. Whether the global interfaces all reside in arith/ or in function/ may have to be decided, or maybe not.

[rwst]

Changed branch from u/rws/factorial_algorithm______does_not_work_as_claimed to none

[rwst]

Dependencies: #24178

[rwst]

Changed author from Ralf Stephan to none

[rwst]

comment:50

The issue was moot because the algorithm keyword is only supported in the arith/ version of factorial/ so just import it.

[videlec]

comment:51

closing positively reviewed duplicates