binomial does not accept variable when only in the lower argument

[sagetrac-Henryk-Trappmann]

sage: var('k')
k 
sage: binomial(x,3)
1/6*(x - 2)*(x - 1)*x
sage: binomial(3,k)
---------------------------------------------------------------------------
TypeError: Either m or x-m must be an integer 

This is a bug since

sage: binomial(x,k)
binomial(x, k)

works.

CC: @kcrisman @jpflori @rwst

Component: symbolics

Author: Burcin Erocal

Branch: d5f5d58

Reviewer: Ralf Stephan

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

[burcin]

make the top level binomial() function symbolic

[burcin]

comment:2

Attachment: trac_9634-symbolic_binomial.patch.gz

attachment: trac_9634-symbolic_binomial.patch replaces the top level binomial() function with the one defined in sage.functions.other. This version can handle symbolic input, as opposed to the previous one from sage.rings.arith.

This still needs work, since the files sage/functions/other.py and sage/rings/arith.py don't pass doctests yet. We need to improve the speed of numerical approximation using the gamma trick from sage.rings.arith.binomial and change sage.symbolic.pynac.py_binomial() to handle large integers.

[burcin]

Author: Burcin Erocal

[kcrisman]

comment:4

Replying to @burcin:

attachment: trac_9634-symbolic_binomial.patch replaces the top level binomial() function with the one defined in sage.functions.other. This version can handle symbolic input, as opposed to the previous one from sage.rings.arith.

Good catch; I was pretty sure we had rewritten that at some point, but I have trouble following the imports.

This still needs work, since the files sage/functions/other.py and sage/rings/arith.py don't pass doctests yet.

We need to improve the speed of numerical approximation using the gamma trick from sage.rings.arith.binomial and change sage.symbolic.pynac.py_binomial() to handle large integers.

Is that part of this ticket? Are you saying that the numerical approximation has slowed down dramatically from the rings.arith version?

[williamstein]

comment:5

This very bug trips up many potential users. Ping. Let's fix it!!

[burcin]

comment:6

Attachment: trac_9634-symbolic_binomial.take2.patch.gz

I uploaded a new patch that fixes doctests as well. This meant reimplementing the _eval_() and _evalf_() methods to override those defined in pynac.

Patchbot, apply only trac_9634-symbolic_binomial.take2.patch.

[fchapoton]

comment:7

for the bot:

apply trac_9634-symbolic_binomial.take2.patch​

[rwst]

Branch: u/rws/ticket/9634

[rwst]

Reviewer: Ralf Stephan

[rwst]

Commit: 7022175

[rwst]

comment:11

The patch2 does not apply cleanly, with two hunks failing, which I fixed manually, and push it to git. However:

sage -t src/sage/functions/other.py  # 18 doctests failed
sage -t src/sage/combinat/partition.py  # 2 doctests failed
sage -t src/sage/rings/arith.py  # 1 doctest failed
sage -t src/sage/symbolic/expression.pyx  # 2 doctests failed

---

New commits:

7022175

trac 9634: Make the top level binomial function symbolic.

[rwst]

comment:12

Never mind, it's all fine, I had an incomplete installation.

[vbraun]

comment:13

merge conflict, please merge the latest beta into this branch.

[sagetrac-git]

Changed commit from 7022175 to 0b0e75e

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

843bf93	Merge branch 'develop' into ticket/9634
0b0e75e	Trac #9870: fix due to merge

[rwst]

comment:15

OK I retested after merge, though only combinat. The number in that commit message is wrong, please ignore.

[vbraun]

comment:16

The branch loses the "Polynomial" global which causes a number of doctest falures

sage -t --long src/sage/rings/polynomial/polynomial_element.pyx
**********************************************************************
File "src/sage/rings/polynomial/polynomial_element.pyx", line 2592, in sage.rings.polynomial.polynomial_element.Polynomial.denominator
Failed example:
    isinstance(x.numerator() / x.denominator(), Polynomial)
Exception raised:
    Traceback (most recent call last):
      File "/scratch/buildbot/sage/redhawk-1/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 480, in _run
        self.execute(example, compiled, test.globs)
      File "/scratch/buildbot/sage/redhawk-1/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 839, in execute
        exec compiled in globs
      File "<doctest sage.rings.polynomial.polynomial_element.Polynomial.denominator[13]>", line 1, in <module>
        isinstance(x.numerator() / x.denominator(), Polynomial)
    NameError: name 'Polynomial' is not defined

[rwst]

comment:17

The funny thing is, it appears the global definition of Polynomial now missing is in power_series_ring_element.pyx which was indirectly and illogically imported in polynomial_ring_element.pyx before the author's patch.

Evidently he should have submitted the import cleanup separately.

Offhand, I'm not able to fix this, any takers?

[jdemeyer]

comment:18

Replying to @rwst:

Evidently he should have submitted the import cleanup separately.

Indeed.

[vbraun]

comment:19

In any case, Polynomial should just be imported into the global namespace in src/sage/rings/polynomial/all.py. The import cleanup is good, even though it should have been separate.

[vbraun]

Changed branch from u/rws/ticket/9634 to u/vbraun/ticket/9634

[vbraun]

New commits:

d5f5d58

import Polynomial into globals

[vbraun]

Changed commit from 0b0e75e to d5f5d58

[vbraun]

Changed branch from u/vbraun/ticket/9634 to d5f5d58

[ppurka]

comment:23

Please follow up in #16726

[ppurka]

Changed commit from d5f5d58 to none