Repository: sympy/sympy

Issue:
Mod shouldn't require integer assumptions
```pytb
>>> (binomial(n, k) % y)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "./sympy/core/decorators.py", line 91, in __sympifyit_wrapper
    return func(a, b)
  File "./sympy/core/decorators.py", line 132, in binary_op_wrapper
    return func(self, other)
  File "./sympy/core/expr.py", line 190, in __mod__
    return Mod(self, other)
  File "./sympy/core/function.py", line 441, in __new__
    result = super(Function, cls).__new__(cls, *args, **options)
  File "./sympy/core/function.py", line 250, in __new__
    evaluated = cls.eval(*args)
  File "./sympy/core/mod.py", line 91, in eval
    rv = doit(p, q)
  File "./sympy/core/mod.py", line 56, in doit
    rv = getattr(p, '_eval_Mod')(q)
  File "./sympy/functions/combinatorial/factorials.py", line 942, in _eval_Mod
ValueError: Integers expected for binomial Mod
```

Functions should only fail on assumptions if they are False, not if they are None. 

This was added in https://github.com/sympy/sympy/pull/14636

CC @sidhantnagpal 

Discussion:

Patch:
diff --git a/sympy/discrete/__init__.py b/sympy/discrete/__init__.py
index 443f52d6dc..0cd8b6753f 100644
--- a/sympy/discrete/__init__.py
+++ b/sympy/discrete/__init__.py
@@ -3,8 +3,7 @@
 Transforms - fft, ifft, ntt, intt, fwht, ifwht,
     mobius_transform, inverse_mobius_transform
 Convolution - convolution, convolution_fft, convolution_ntt, convolution_fwht,
-    convolution_subset, covering_product, intersecting_product
-Recurrence - linrec
+    convolution_subset
 """
 
 from .transforms import (fft, ifft, ntt, intt, fwht, ifwht,
diff --git a/sympy/discrete/convolution.py b/sympy/discrete/convolution.py
index d8a7f5d542..01459ee942 100644
--- a/sympy/discrete/convolution.py
+++ b/sympy/discrete/convolution.py
@@ -1,6 +1,5 @@
 """
-Convolution (using FFT, NTT, FWHT), Subset Convolution,
-Covering Product, Intersecting Product
+Convolution (using FFT, NTT, FWHT), Subset Convolution
 """
 from __future__ import print_function, division
 
@@ -11,12 +10,16 @@
     fft, ifft, ntt, intt, fwht, ifwht)
 
 
-def convolution(a, b, **hints):
+def convolution(a, b, cycle=0, dps=None, prime=None, dyadic=None, subset=None):
     """
     Performs convolution by determining the type of desired
     convolution using hints.
 
-    If no hints are given, linear convolution is performed using
+    Exactly one of `dps`, `prime`, `dyadic`, `subset` arguments should be
+    specified explicitly for identifying the type of convolution, and the
+    argument `cycle` can be specified optionally.
+
+    For the default arguments, linear convolution is performed using
     FFT.
 
     Parameters
@@ -24,22 +27,19 @@ def convolution(a, b, **hints):
 
     a, b : iterables
         The sequences for which convolution is performed.
-    hints : dict
-        Specifies the type of convolution to be performed.
-        The following hints can be given as keyword arguments.
-        dps : Integer
-            Specifies the number of decimal digits for precision for
-            performing FFT on the sequence.
-        prime : Integer
-            Prime modulus of the form (m*2**k + 1) to be used for
-            performing NTT on the sequence.
-        cycle : Integer
-            Specifies the length for doing cyclic convolution.
-        dyadic : bool
-            Identifies the convolution type as dyadic (XOR)
-            convolution, which is performed using FWHT.
-        subset : bool
-            Identifies the convolution type as subset convolution.
+    cycle : Integer
+        Specifies the length for doing cyclic convolution.
+    dps : Integer
+        Specifies the number of decimal digits for precision for
+        performing FFT on the sequence.
+    prime : Integer
+        Prime modulus of the form (m*2**k + 1) to be used for
+        performing NTT on the sequence.
+    dyadic : bool
+        Identifies the convolution type as dyadic (XOR)
+        convolution, which is performed using FWHT.
+    subset : bool
+        Identifies the convolution type as subset convolution.
 
     Examples
     ========
@@ -69,29 +69,18 @@ def convolution(a, b, **hints):
 
     """
 
-    fft = hints.pop('fft', None)
-    dps = hints.pop('dps', None)
-    p = hints.pop('prime', None)
-    c = as_int(hints.pop('cycle', 0))
-    dyadic = hints.pop('dyadic', None)
-    subset = hints.pop('subset', None)
-
+    c = as_int(cycle)
     if c < 0:
         raise ValueError("The length for cyclic convolution must be non-negative")
 
-    fft = True if fft else None
     dyadic = True if dyadic else None
     subset = True if subset else None
-    if sum(x is not None for x in (p, dps, dyadic, subset)) > 1 or \
-            sum(x is not None for x in (fft, dyadic, subset)) > 1:
-        raise TypeError("Ambiguity in determining the convolution type")
-
-    if p is not None:
-        ls = convolution_ntt(a, b, prime=p)
-        return ls if not c else [sum(ls[i::c]) % p for i in range(c)]
+    if sum(x is not None for x in (prime, dps, dyadic, subset)) > 1:
+        raise TypeError("Ambiguity in determining the type of convolution")
 
-    elif hints.pop('ntt', False):
-        raise TypeError("Prime modulus must be specified for performing NTT")
+    if prime is not None:
+        ls = convolution_ntt(a, b, prime=prime)
+        return ls if not c else [sum(ls[i::c]) % prime for i in range(c)]
 
     if dyadic:
         ls = convolution_fwht(a, b)
diff --git a/sympy/functions/combinatorial/factorials.py b/sympy/functions/combinatorial/factorials.py
index 610a16e783..ec6ddea6d1 100644
--- a/sympy/functions/combinatorial/factorials.py
+++ b/sympy/functions/combinatorial/factorials.py
@@ -894,7 +894,7 @@ def eval(cls, n, k):
     def _eval_Mod(self, q):
         n, k = self.args
 
-        if not all(x.is_integer for x in (n, k, q)):
+        if any(x.is_integer is False for x in (n, k, q)):
             raise ValueError("Integers expected for binomial Mod")
 
         if all(x.is_Integer for x in (n, k, q)):


Test patch:
diff --git a/sympy/discrete/tests/test_convolution.py b/sympy/discrete/tests/test_convolution.py
index 1b1bf6d568..8036cbbb1d 100644
--- a/sympy/discrete/tests/test_convolution.py
+++ b/sympy/discrete/tests/test_convolution.py
@@ -16,9 +16,9 @@ def test_convolution():
     c = [3, 5, 3, 7, 8]
     d = [1422, 6572, 3213, 5552]
 
-    assert convolution(a, b, fft=True) == convolution_fft(a, b)
-    assert convolution(a, b, dps=9, fft=True) == convolution_fft(a, b, dps=9)
-    assert convolution(a, d, fft=True, dps=7) == convolution_fft(d, a, dps=7)
+    assert convolution(a, b) == convolution_fft(a, b)
+    assert convolution(a, b, dps=9) == convolution_fft(a, b, dps=9)
+    assert convolution(a, d, dps=7) == convolution_fft(d, a, dps=7)
     assert convolution(a, d[1:], dps=3) == convolution_fft(d[1:], a, dps=3)
 
     # prime moduli of the form (m*2**k + 1), sequence length
@@ -27,35 +27,26 @@ def test_convolution():
     q = 19*2**10 + 1
 
     # ntt
-    assert convolution(d, b, ntt=True, prime=q) == convolution_ntt(b, d, prime=q)
+    assert convolution(d, b, prime=q) == convolution_ntt(b, d, prime=q)
     assert convolution(c, b, prime=p) == convolution_ntt(b, c, prime=p)
-    assert convolution(d, c, prime=p, ntt=True) == convolution_ntt(c, d, prime=p)
-    raises(TypeError, lambda: convolution(b, d, ntt=True))
-    raises(TypeError, lambda: convolution(b, d, ntt=True, cycle=0))
+    assert convolution(d, c, prime=p) == convolution_ntt(c, d, prime=p)
     raises(TypeError, lambda: convolution(b, d, dps=5, prime=q))
-    raises(TypeError, lambda: convolution(b, d, dps=6, ntt=True, prime=q))
-    raises(TypeError, lambda: convolution(b, d, fft=True, dps=7, ntt=True, prime=q))
-    # ntt is a specialized variant of fft, TypeError should not be raised
-    assert convolution(b, d, fft=True, ntt=True, prime=q) == \
-            convolution_ntt(b, d, prime=q)
+    raises(TypeError, lambda: convolution(b, d, dps=6, prime=q))
 
     # fwht
     assert convolution(a, b, dyadic=True) == convolution_fwht(a, b)
     assert convolution(a, b, dyadic=False) == convolution(a, b)
-    raises(TypeError, lambda: convolution(b, d, fft=True, dps=2, dyadic=True))
-    raises(TypeError, lambda: convolution(b, d, ntt=True, prime=p, dyadic=True))
-    raises(TypeError, lambda: convolution(b, d, fft=True, dyadic=True))
+    raises(TypeError, lambda: convolution(b, d, dps=2, dyadic=True))
+    raises(TypeError, lambda: convolution(b, d, prime=p, dyadic=True))
     raises(TypeError, lambda: convolution(a, b, dps=2, dyadic=True))
     raises(TypeError, lambda: convolution(b, c, prime=p, dyadic=True))
 
     # subset
     assert convolution(a, b, subset=True) == convolution_subset(a, b) == \
             convolution(a, b, subset=True, dyadic=False) == \
-                convolution(a, b, subset=True, fft=False) == \
-                    convolution(a, b, subset=True, fft=False, ntt=False)
+                convolution(a, b, subset=True)
     assert convolution(a, b, subset=False) == convolution(a, b)
     raises(TypeError, lambda: convolution(a, b, subset=True, dyadic=True))
-    raises(TypeError, lambda: convolution(b, c, subset=True, fft=True))
     raises(TypeError, lambda: convolution(c, d, subset=True, dps=6))
     raises(TypeError, lambda: convolution(a, c, subset=True, prime=q))
 
@@ -72,12 +63,6 @@ def test_cyclic_convolution():
 
     assert convolution([1, 2, 3], [4, 5, 6], cycle=3) == [31, 31, 28]
 
-    assert convolution(a, b, fft=True, cycle=4) == \
-            convolution(a, b, cycle=4)
-
-    assert convolution(a, b, fft=True, dps=3, cycle=4) == \
-            convolution(a, b, dps=3, cycle=4)
-
     a = [S(1)/3, S(7)/3, S(5)/9, S(2)/7, S(5)/8]
     b = [S(3)/5, S(4)/7, S(7)/8, S(8)/9]
 
diff --git a/sympy/functions/combinatorial/tests/test_comb_factorials.py b/sympy/functions/combinatorial/tests/test_comb_factorials.py
index b02b126673..10ecf2dc0d 100644
--- a/sympy/functions/combinatorial/tests/test_comb_factorials.py
+++ b/sympy/functions/combinatorial/tests/test_comb_factorials.py
@@ -401,6 +401,11 @@ def test_binomial_Mod():
     p, q = 10**5 + 3, 10**9 + 33 # prime modulo
     r, s = 10**7 + 5, 33333333 # composite modulo
 
+    n, k, m = symbols('n k m')
+    assert (binomial(n, k) % q).subs({n: s, k: p}) == Mod(binomial(s, p), q)
+    assert (binomial(n, k) % m).subs({n: 8, k: 5, m: 13}) == 4
+    assert (binomial(9, k) % 7).subs(k, 2) == 1
+
     # Lucas Theorem
     assert Mod(binomial(156675, 4433, evaluate=False), p) == Mod(binomial(156675, 4433), p)
     assert Mod(binomial(123456, 43253, evaluate=False), p) == Mod(binomial(123456, 43253), p)