sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.37k stars 465 forks source link

Pochhammer symbols #19461

Open rwst opened 8 years ago

rwst commented 8 years ago

While Sage already has falling_factorial and rising_factorial functions they are not symbolic. Always using gamma or product expansions may be inconvenient in some cases, and does not offer the option to rewrite expressions in terms of them. Especially the product form cannot be easily rewritten as gamma expression. So the product expansion should be made optional.

Component: symbolics

Keywords: falling_factorial, rising_factorial

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

arminstraub commented 8 years ago
comment:1

It seems that this has been fixed in recent versions of Sage. At least, the functions can now be used as part of symbolic expressions:

sage: rising_factorial(x, 3)
(x + 2)*(x + 1)*x
sage: rising_factorial(x, x)
gamma(2*x)/gamma(x)

I have therefore set the ticket to "invalid". Please change back if I am misreading the description.

arminstraub commented 8 years ago
comment:2

On another ticket I was told to also set these to positive review... If that's not universally true, please let me know.

arminstraub commented 8 years ago

Reviewer: Armin Straub

rwst commented 8 years ago
comment:3

Replying to @arminstraub:

It seems that this has been fixed in recent versions of Sage. At least, the functions can now be used as part of symbolic expressions:

sage: rising_factorial(x, 3)
(x + 2)*(x + 1)*x
sage: rising_factorial(x, x)
gamma(2*x)/gamma(x)

It was not fixed. I would like at least the option of not converting to gamma fractions, which is impossible without a symbolic function. Also, you don't want rising_factorial(x, 3000) expanded. The function itself is worthwhile to have.

On another ticket I was told to also set these to positive review.

I think this applies only to tickets that you have started.

rwst commented 8 years ago

Changed reviewer from Armin Straub to none

arminstraub commented 8 years ago
comment:4

Replying to @rwst:

Also, you don't want rising_factorial(x, 3000) expanded.

I would disagree on that part. If I ever do explicitly write rising_factorial(x, 3000), then I would like it expanded (just as I appreciate that factorial(3000) returns a huge integer). For what it's worth, that is what Mathematica does.

Also note that we can express the Pochhammers using binomial coefficents. And, binomial(x,3000) (which equals falling_factorial(x,3000)/factorial(3000)) is returned in expanded form (and I am glad it is). The worst alternative would be a change in behaviour at a random value, which is deemed "too large" for expansion.

The function itself is worthwhile to have.

That's of course a different matter. Maybe you could update the description? (The part after "i.e." and the second sentence about implementations like Gosper do not apply anymore.)

On another ticket I was told to also set these to positive review.

I think this applies only to tickets that you have started.

I see, thanks!

rwst commented 8 years ago

Description changed:

--- 
+++ 
@@ -1 +1 @@
-While Sage already has `falling_factorial` and `rising_factorial` functions they are not symbolic, i.e. cannot be used as part of symbolic expressions. This prevents working with hypergeometric expressions and implementation of e.g. Gosper's algorithm.
+While Sage already has `falling_factorial` and `rising_factorial` functions they are not symbolic. Always using gamma or product expansions may be inconvenient in some cases, and does not offer the option to rewrite expressions in terms of them.
pelegm commented 7 years ago

Changed keywords from none to falling_factorial, rising_factorial

rwst commented 7 years ago
comment:7

Replying to @arminstraub:

Maybe you could update the description? (The part after "i.e." and the second sentence about implementations like Gosper do not apply anymore.)

Actually it does apply. Gosper's and similar algorithms need to transform expressions to gamma expressions, and that's very simple with a rising_factorial(x,5). The information is lost with (x + 4)*(x + 3)*(x + 2)*(x + 1)*x.

rwst commented 7 years ago

Description changed:

--- 
+++ 
@@ -1 +1 @@
-While Sage already has `falling_factorial` and `rising_factorial` functions they are not symbolic. Always using gamma or product expansions may be inconvenient in some cases, and does not offer the option to rewrite expressions in terms of them.
+While Sage already has `falling_factorial` and `rising_factorial` functions they are not symbolic. Always using gamma or product expansions may be inconvenient in some cases, and does not offer the option to rewrite expressions in terms of them. Especially the product form cannot be easily rewritten as gamma expression. So the product expansion should be made optional.
rwst commented 7 years ago
comment:9

The planned behaviour would be expanding as the default. Adding hold=True will prevent it.

However, it seems at first the ticket cannot be implemented unless matrices etc. coerce into SR:

File "src/sage/arith/misc.py", line 4377, in sage.arith.misc.falling_factorial
Failed example:
    falling_factorial(A, 2) # A(A - I)
...
      File "sage/symbolic/function.pyx", line 996, in sage.symbolic.function.BuiltinFunction.__call__ (build/cythonized/sage/symbolic/function.cpp:11436)
        res = super(BuiltinFunction, self).__call__(
      File "sage/symbolic/function.pyx", line 474, in sage.symbolic.function.Function.__call__ (build/cythonized/sage/symbolic/function.cpp:6321)
        raise TypeError("cannot coerce arguments: %s" % (err))
    TypeError: cannot coerce arguments: no canonical coercion from Full MatrixSpace of 4 by 4 dense matrices over Integer Ring to Symbolic Ring

The reason is that making the symbolic rising/falling_factorial the default will switch on some checks that are in symbolic/function.pyx so the doctests in arith/misc.py will fail unless the arith/ version is explicitly called, which is a viable option IMHO. Compare #17489 which is stuck because the overriding of the arith/ version (of factorial()) causes failure of the algorithm keyword.

The pragmatic solution would be to accept there are two versions of the functions rising/falling_/factorial() (more?) and that the versions in arith/ must be explicitly called if the symbolic ones don't suffice.