Skip to content

Commit b159b4b

Browse files
author
Release Manager
committed
gh-39891: Speed up binomial As first observed in #39379, when `n` and `k` are small sage integers, `binomial(n, k)` are much slower than `n.binomial(k)`. This pull request fixes it. Now `%timeit binomial(20, 10)` is only twice slower than `%timeit 20.binomial(10)` — previously it was **30** times slower. There's also a minor patch where some code is skipped when all arguments are instances of `Element`. It is certainly a more common case for the input to be `Element` than otherwise, so we optimize for it. Another minor patch involves storing imported modules as C global variables instead of re-import every time. I think using not-overriding `_method_arguments` as an indicator of "don't call the method" (then silently catch `TypeError` and throw it away) is rather bad design, but this is not in scope here. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. URL: #39891 Reported by: user202729 Reviewer(s): Travis Scrimshaw
2 parents 8e4b772 + 3cf1002 commit b159b4b

File tree

5 files changed

+39
-14
lines changed

5 files changed

+39
-14
lines changed

src/sage/arith/misc.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3959,7 +3959,7 @@ def binomial(x, m, **kwds):
39593959
P = parent(x)
39603960
x = py_scalar_to_element(x)
39613961

3962-
# case 1: native binomial implemented on x
3962+
# case 1: native binomial implemented on x (see also dont_call_method_on_arg)
39633963
try:
39643964
return P(x.binomial(m, **kwds))
39653965
except (AttributeError, TypeError):

src/sage/functions/other.py

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1715,6 +1715,17 @@ def _binomial_sym(self, n, k):
17151715
from sage.misc.misc_c import prod
17161716
return prod(n - i for i in range(k)) / factorial(k)
17171717

1718+
def _method_arguments(self, n, k):
1719+
"""
1720+
See :meth:`sage.symbolic.function.BuiltinFunction._method_arguments`.
1721+
1722+
TESTS::
1723+
1724+
sage: binomial._method_arguments(10, 5)
1725+
(10, 5)
1726+
"""
1727+
return (n, k)
1728+
17181729
def _eval_(self, n, k):
17191730
"""
17201731
EXAMPLES::

src/sage/functions/transcendental.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -390,7 +390,7 @@ def _evalf_(self, n, x, parent=None, algorithm=None):
390390
"""
391391
return _mpmath_utils_call(_mpmath_zeta, x, 1, n, parent=parent)
392392

393-
def _method_arguments(self, k, x, **args):
393+
def _method_arguments(self, k, x):
394394
r"""
395395
TESTS::
396396

src/sage/symbolic/function.pxd

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,20 +3,20 @@ from sage.structure.sage_object cimport SageObject
33
cdef class Function(SageObject):
44
cdef unsigned int _serial
55
cdef int _nargs
6-
cdef object _name
7-
cdef object _alt_name
8-
cdef object _latex_name
6+
cdef str _name
7+
cdef str _alt_name
8+
cdef str _latex_name
99
cdef object _conversions
1010
cdef object _evalf_params_first
1111
cdef _is_registered(self)
1212
cdef _register_function(self)
1313

1414
cdef class BuiltinFunction(Function):
15-
cdef object _preserved_arg
15+
cdef int _preserved_arg # 0 if none, otherwise in [1.._nargs], see function.pyx
1616
cdef _is_registered(self)
1717

1818
cdef class GinacFunction(BuiltinFunction):
19-
cdef object _ginac_name
19+
cdef str _ginac_name
2020
cdef _is_registered(self)
2121
cdef _register_function(self)
2222

src/sage/symbolic/function.pyx

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,7 @@ from sage.structure.sage_object cimport SageObject
141141
from sage.structure.element cimport Element, parent, Expression
142142
from sage.misc.lazy_attribute import lazy_attribute
143143

144+
from sage.structure.parent cimport Parent
144145
from sage.structure.coerce cimport (coercion_model,
145146
py_scalar_to_element, is_numpy_type, is_mpmath_type)
146147
from sage.structure.richcmp cimport richcmp
@@ -157,6 +158,7 @@ try:
157158
except ImportError:
158159
register_or_update_function = None
159160

161+
cdef object SR = None, PolynomialRing_commutative = None, MPolynomialRing_polydict_domain = None
160162

161163
# List of functions which ginac allows us to define custom behavior for.
162164
# Changing the order of this list could cause problems unpickling old pickles.
@@ -886,7 +888,7 @@ cdef class BuiltinFunction(Function):
886888
sage: c(pi/2) # needs sage.symbolic
887889
0
888890
"""
889-
self._preserved_arg = preserved_arg
891+
self._preserved_arg = 0 if preserved_arg is None else preserved_arg
890892
if preserved_arg and (preserved_arg < 1 or preserved_arg > nargs):
891893
raise ValueError("preserved_arg must be between 1 and nargs")
892894

@@ -913,6 +915,13 @@ cdef class BuiltinFunction(Function):
913915
univariate functions. Multivariate symbolic functions should override
914916
it as appropriate.
915917
918+
Note that it is mandatory for multivariate symbolic functions to
919+
override this function in order to allow delegating to the method
920+
implemented on the element.
921+
For example, ``binomial(n, k)`` tries to call ``n.binomial(k)``
922+
because :meth:`sage.functions.other.Function_binomial._method_arguments`
923+
is implemented.
924+
916925
EXAMPLES::
917926
918927
sage: zeta._method_arguments(1)
@@ -983,7 +992,7 @@ cdef class BuiltinFunction(Function):
983992
'foo'
984993
"""
985994
res = None
986-
if args and not hold:
995+
if args and not hold and not all(isinstance(arg, Element) for arg in args):
987996
# try calling the relevant math, cmath, mpmath or numpy function.
988997
# And as a fallback try the custom self._eval_numpy_ or
989998
# self._eval_mpmath_
@@ -1046,17 +1055,20 @@ cdef class BuiltinFunction(Function):
10461055
res = super().__call__(
10471056
*args, coerce=coerce, hold=hold)
10481057

1049-
# Convert the output back to the corresponding
1050-
# Python type if possible.
1058+
cdef Parent arg_parent
10511059
if any(isinstance(x, Element) for x in args):
10521060
if (self._preserved_arg
10531061
and isinstance(args[self._preserved_arg-1], Element)):
10541062
arg_parent = parent(args[self._preserved_arg-1])
1055-
from sage.symbolic.ring import SR
1063+
global SR
1064+
if SR is None:
1065+
from sage.symbolic.ring import SR
10561066
if arg_parent is SR:
10571067
return res
1058-
from sage.rings.polynomial.polynomial_ring import PolynomialRing_commutative
1059-
from sage.rings.polynomial.multi_polynomial_ring import MPolynomialRing_polydict_domain
1068+
global PolynomialRing_commutative, MPolynomialRing_polydict_domain
1069+
if PolynomialRing_commutative is None:
1070+
from sage.rings.polynomial.polynomial_ring import PolynomialRing_commutative
1071+
from sage.rings.polynomial.multi_polynomial_ring import MPolynomialRing_polydict_domain
10601072
if isinstance(arg_parent, (PolynomialRing_commutative,
10611073
MPolynomialRing_polydict_domain)):
10621074
try:
@@ -1072,6 +1084,8 @@ cdef class BuiltinFunction(Function):
10721084
if not isinstance(res, Element):
10731085
return res
10741086

1087+
# Convert the output back to the corresponding
1088+
# Python type if possible.
10751089
p = res.parent()
10761090
from sage.rings.complex_double import CDF
10771091
from sage.rings.integer_ring import ZZ

0 commit comments

Comments
 (0)