Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use the faster binomial in combinat #39385

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

use the faster binomial in combinat #39385

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bb8b646
use the faster binomial in combinat
fchapoton Jan 27, 2025
40d0e60
using Integer more
fchapoton Jan 28, 2025
07517dd
code detail
fchapoton Jan 31, 2025
db52fb0
other code details
fchapoton Jan 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 66 additions & 67 deletions src/sage/combinat/affine_permutation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
# ****************************************************************************
from itertools import repeat

from sage.arith.misc import binomial
from sage.categories.affine_weyl_groups import AffineWeylGroups
from sage.combinat.composition import Composition
from sage.combinat.partition import Partition
Expand All @@ -23,7 +22,7 @@
from sage.misc.lazy_import import lazy_import
from sage.misc.misc_c import prod
from sage.misc.prandom import randint
from sage.rings.integer_ring import ZZ
from sage.rings.integer import Integer
from sage.structure.list_clone import ClonableArray
from sage.structure.parent import Parent
from sage.structure.unique_representation import UniqueRepresentation
Expand Down Expand Up @@ -76,14 +75,14 @@
Type A affine permutation with window [1, 2, 3, 4]
"""
if check:
lst = [ZZ(val) for val in lst]
lst = [Integer(val) for val in lst]
self.k = parent.k
self.n = self.k + 1
#This N doesn't matter for type A, but comes up in all other types.
# This N doesn't matter for type A, but comes up in all other types.
if parent.cartan_type()[0] == 'A':
self.N = self.n
elif parent.cartan_type()[0] in ['B', 'C', 'D']:
self.N = 2*self.k + 1
self.N = 2 * self.k + 1
elif parent.cartan_type()[0] == 'G':
self.N = 6
else:
Expand Down Expand Up @@ -245,7 +244,7 @@
"""
return self == self.parent().one() or self.descents(side) == [i]

def index_set(self):
def index_set(self) -> tuple:
r"""
Index set of the affine permutation group.

Expand All @@ -255,7 +254,7 @@
sage: A.index_set()
(0, 1, 2, 3, 4, 5, 6, 7)
"""
return tuple(range(self.k+1))
return tuple(range(self.k + 1))

Check warning on line 257 in src/sage/combinat/affine_permutation.py

View check run for this annotation

Codecov / codecov/patch

src/sage/combinat/affine_permutation.py#L257 

Added line #L257 was not covered by tests

def lower_covers(self, side='right'):
r"""
Expand Down Expand Up @@ -305,7 +304,7 @@
sage: p.reduced_word()
[0, 7, 4, 1, 0, 7, 5, 4, 2, 1]
"""
#This is about 25% faster than the default algorithm.
# This is about 25% faster than the default algorithm.
x = self
i = 0
word = []
Expand Down Expand Up @@ -410,10 +409,10 @@


class AffinePermutationTypeA(AffinePermutation):
#----------------------
#Type-specific methods.
#(Methods existing in all types, but with type-specific definition.)
#----------------------
# ----------------------
# Type-specific methods.
# (Methods existing in all types, but with type-specific definition.)
# ----------------------
def check(self):
r"""
Check that ``self`` is an affine permutation.
Expand All @@ -440,13 +439,14 @@
if not self:
return
k = self.parent().k
#Type A.
# Type A
if len(self) != k + 1:
raise ValueError("length of list must be k+1="+str(k+1))
if binomial(k+2,2) != sum(self):
raise ValueError("window does not sum to " + str(binomial((k+2),2)))
l = sorted([i % (k+1) for i in self])
if l != list(range(k+1)):
raise ValueError(f"length of list must be k+1={k + 1}")
sigma = (k + 2).binomial(2)
if sigma != sum(self):
raise ValueError(f"window does not sum to {sigma}")
l = sorted(i % (k + 1) for i in self)
if any(i != j for i, j in enumerate(l)):
raise ValueError("entries must have distinct residues")

def value(self, i, base_window=False):
Expand Down Expand Up @@ -510,11 +510,11 @@
Type A affine permutation with window [3, -1, 6, 0, 5, 4, 10, 9]
"""
j = i % (self.k+1)
#Cloning is currently kinda broken, in that caches don't clear which
#leads to strangeness with the cloned object.
#The clone approach is quite a bit (2x) faster, though, so this should
#switch once the caching situation is fixed.
#with self.clone(check=False) as l:
# Cloning is currently kinda broken, in that caches don't clear which
# leads to strangeness with the cloned object.
# The clone approach is quite a bit (2x) faster, though, so this should
# switch once the caching situation is fixed.
# with self.clone(check=False) as l:
l = self[:]
if j == 0:
a = l[0]
Expand All @@ -524,7 +524,6 @@
a = l[j-1]
l[j-1] = l[j]
l[j] = a
#return l
return type(self)(self.parent(), l, check=False)

def apply_simple_reflection_left(self, i):
Expand All @@ -539,18 +538,18 @@
sage: p.apply_simple_reflection_left(11)
Type A affine permutation with window [4, -1, 0, 6, 5, 3, 10, 9]
"""
#Here are a couple other methods we tried out, but turned out
#to be slower than the current implementation.
#1) This one was very bad:
# return self.inverse().apply_simple_reflection_right(i).inverse()
#2) Also bad, though not quite so bad:
# return (self.parent().simple_reflection(i))*self
i = i % (self.k+1)
#Cloning is currently kinda broken, in that caches don't clear which
#leads to strangeness with the cloned object.
#The clone approach is quite a bit faster, though, so this should switch
#once the caching situation is fixed.
#with self.clone(check=False) as l:
# Here are a couple other methods we tried out, but turned out
# to be slower than the current implementation.
# 1) This one was very bad:
# return self.inverse().apply_simple_reflection_right(i).inverse()
# 2) Also bad, though not quite so bad:
# return (self.parent().simple_reflection(i))*self
i = i % (self.k + 1)
# Cloning is currently kinda broken, in that caches don't clear which
# leads to strangeness with the cloned object.
# The clone approach is quite a bit faster, though,
# so this should switch once the caching situation is fixed.
# with self.clone(check=False) as l:
l = []
if i != self.k:
for m in range(self.k + 1):
Expand Down Expand Up @@ -627,10 +626,10 @@
"""
return self

#----------------------
#Type-A-specific methods.
#Only available in Type A.
#----------------------
# ----------------------
# Type-A-specific methods.
# Only available in Type A.
# ----------------------

def flip_automorphism(self):
r"""
Expand Down Expand Up @@ -699,7 +698,7 @@
else:
descents = self.descents(side='left')
side = 'left'
#for now, assume side is 'right')
# for now, assume side is 'right')
best_T = []
for i in descents:
y = self.clone().apply_simple_reflection(i,side)
Expand Down Expand Up @@ -834,18 +833,18 @@
"""
code = [0 for i in range(self.k+1)]
if typ[0] == 'i' and side[0] == 'r':
#Find number of positions to the right of position i with smaller
#value than the number in position i.
# Find number of positions to the right of position i with smaller
# value than the number in position i.
for i in range(self.k+1):
a = self(i)
for j in range(i+1, i+self.k+1):
b = self(j)
if b < a:
code[i] += (a-b) // (self.k+1) + 1
elif typ[0] == 'd' and side[0] == 'r':
#Find number of positions to the left of position i with larger
#value than the number in position i. Then cyclically shift
#the resulting vector.
# Find number of positions to the left of position i with larger
# value than the number in position i. Then cyclically shift
# the resulting vector.
for i in range(self.k+1):
a = self(i)
for j in range(i-self.k, i):
Expand All @@ -855,18 +854,18 @@
if a < b:
code[i-1] += ((b-a)//(self.k+1)+1)
elif typ[0] == 'i' and side[0] == 'l':
#Find number of positions to the right of i smaller than i, then
#cyclically shift the resulting vector.
# Find number of positions to the right of i smaller than i, then
# cyclically shift the resulting vector.
for i in range(self.k+1):
pos = self.position(i)
for j in range(pos+1, pos+self.k+1):
b = self(j)
#A small rotation is necessary for the reduced word from
#the lehmer code to match the element.
# A small rotation is necessary for the reduced word from
# the lehmer code to match the element.
if b < i:
code[i-1] += (i-b) // (self.k+1) + 1
elif typ[0] == 'd' and side[0] == 'l':
#Find number of positions to the left of i larger than i.
# Find number of positions to the left of i larger than i.
for i in range(self.k+1):
pos = self.position(i)
for j in range(pos-self.k, pos):
Expand Down Expand Up @@ -1103,7 +1102,7 @@

sage: C = AffinePermutationGroup(['C',4,1])
sage: x = C.one()
sage: [x.value(i) for i in range(-10,10)] == list(range(-10,10))
sage: all(x.value(i) == i for i in range(-10,10))
True
"""
N = 2*self.k + 1
Expand All @@ -1124,7 +1123,7 @@

sage: C = AffinePermutationGroup(['C',4,1])
sage: x = C.one()
sage: [x.position(i) for i in range(-10,10)] == list(range(-10,10))
sage: all(x.position(i) == i for i in range(-10,10))
True
"""
N = 2*self.k + 1
Expand Down Expand Up @@ -2001,9 +2000,9 @@
methods for the specific affine permutation groups.
"""

#----------------------
#Type-free methods.
#----------------------
# ----------------------
# Type-free methods.
# ----------------------

def __init__(self, cartan_type):
r"""
Expand All @@ -2014,13 +2013,13 @@
"""
Parent.__init__(self, category=AffineWeylGroups())
ct = CartanType(cartan_type)
self.k = ct.n
self.k = Integer(ct.n)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as earlier: from my reading of the code, ct.n will always be a Sage integer, so this cast should be unnecessary. Is there any reason you have Integer(ct.n) here instead of ct.n? I did some timing experiments and calling Integer does slow things down, and from the PR title this is supposed to be an optimization PR.

There are a few other locations in the code where you're calling Integer on something that I think is already going to be a Sage integer.

self.n = ct.rank()
#This N doesn't matter for type A, but comes up in all other types.
# This N doesn't matter for type A, but comes up in all other types.
if ct.letter == 'A':
self.N = self.k + 1
elif ct.letter == 'B' or ct.letter == 'C' or ct.letter == 'D':
self.N = 2*self.k + 1
self.N = 2 * self.k + 1
elif ct.letter == 'G':
self.N = 6
self._cartan_type = ct
Expand All @@ -2034,14 +2033,14 @@
"""
return self.element_class(self, *args, **keywords)

def _repr_(self):
def _repr_(self) -> str:
r"""
TESTS::

sage: AffinePermutationGroup(['A',7,1])
The group of affine permutations of type ['A', 7, 1]
"""
return "The group of affine permutations of type "+str(self.cartan_type())
return "The group of affine permutations of type " + str(self.cartan_type())

def _test_enumeration(self, n=4, **options):
r"""
Expand Down Expand Up @@ -2242,12 +2241,12 @@
True
sage: TestSuite(A).run()
"""
return self(list(range(1, self.k + 2)))
return self(range(1, self.k + 2))

#------------------------
#Type-unique methods.
#(Methods which do not exist in all types.)
#------------------------
# ------------------------
# Type-unique methods.
# (Methods which do not exist in all types.)
# ------------------------
def from_lehmer_code(self, C, typ='decreasing', side='right'):
r"""
Return the affine permutation with the supplied Lehmer code (a weak
Expand Down Expand Up @@ -2353,7 +2352,7 @@
True
sage: TestSuite(C).run()
"""
return self(list(range(1, self.k + 1)))
return self(range(1, self.k + 1))

Element = AffinePermutationTypeC

Expand Down
28 changes: 13 additions & 15 deletions src/sage/combinat/baxter_permutations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
"""
Baxter permutations
"""

from sage.structure.unique_representation import UniqueRepresentation
from sage.structure.parent import Parent
from sage.sets.disjoint_union_enumerated_sets import DisjointUnionEnumeratedSets
from sage.combinat.permutation import Permutations

from sage.rings.integer import Integer
from sage.rings.integer_ring import ZZ
from sage.sets.disjoint_union_enumerated_sets import DisjointUnionEnumeratedSets
from sage.structure.parent import Parent
from sage.structure.unique_representation import UniqueRepresentation


class BaxterPermutations(UniqueRepresentation, Parent):
Expand Down Expand Up @@ -79,11 +78,11 @@ def __init__(self, n):
Baxter permutations of size 5
"""
self.element_class = Permutations(n).element_class
self._n = ZZ(n)
self._n = Integer(n)
from sage.categories.finite_enumerated_sets import FiniteEnumeratedSets
super().__init__(category=FiniteEnumeratedSets())

def _repr_(self):
def _repr_(self) -> str:
"""
Return a string representation of ``self``.

Expand All @@ -93,9 +92,9 @@ def _repr_(self):
sage: BaxterPermutations_size(5)
Baxter permutations of size 5
"""
return "Baxter permutations of size %s" % self._n
return f"Baxter permutations of size {self._n}"

def __contains__(self, x):
def __contains__(self, x) -> bool:
r"""
Return ``True`` if and only if ``x`` is a Baxter permutation of
size ``self._n``.
Expand Down Expand Up @@ -228,12 +227,11 @@ def cardinality(self):
Integer Ring
"""
if self._n == 0:
return 1
from sage.arith.misc import binomial
return sum((binomial(self._n + 1, k) *
binomial(self._n + 1, k + 1) *
binomial(self._n + 1, k + 2)) //
((self._n + 1) * binomial(self._n + 1, 2))
return ZZ.one()
n = self._n + 1
return sum((n.binomial(k) *
n.binomial(k + 1) *
n.binomial(k + 2)) // (n * n.binomial(2))
for k in range(self._n))


Expand Down
Loading
Loading