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

Jump to bottom

Decouple tuple #35812

Merged
merged 8 commits into from
Jul 9, 2023
Merged

Decouple tuple #35812

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e057b27
Mind the GAP
deinst Jun 22, 2023
aba3954
Finish decoupling tuple.py from GAP.
deinst Jun 22, 2023
5702830
Added testcases to Tuples for undelying lists with duplicates
deinst Jun 23, 2023
dffe683
Have tuple.py return tuples instead of lists
deinst Jun 25, 2023
14e7540
Switch tuple to list in projective_space.py
deinst Jun 25, 2023
f40c739
Merge branch 'develop' into decouple_tuple
deinst Jun 26, 2023
660653c
Change dict to set
deinst Jun 26, 2023
f79c0d0
Merge tag '10.1.beta5' into decouple_tuple
mkoeppe Jul 2, 2023
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
88 changes: 39 additions & 49 deletions src/sage/combinat/tuple.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@
# https://www.gnu.org/licenses/
# ****************************************************************************

from sage.libs.gap.libgap import libgap
from sage.arith.misc import binomial
from sage.rings.integer_ring import ZZ
from sage.structure.parent import Parent
from sage.structure.unique_representation import UniqueRepresentation
from sage.categories.finite_enumerated_sets import FiniteEnumeratedSets

from itertools import product, combinations_with_replacement

class Tuples(Parent, UniqueRepresentation):
"""
Expand All @@ -35,23 +35,23 @@ class Tuples(Parent, UniqueRepresentation):

sage: S = [1,2]
sage: Tuples(S,3).list()
[[1, 1, 1], [2, 1, 1], [1, 2, 1], [2, 2, 1], [1, 1, 2],
[2, 1, 2], [1, 2, 2], [2, 2, 2]]
[(1, 1, 1), (2, 1, 1), (1, 2, 1), (2, 2, 1), (1, 1, 2),
(2, 1, 2), (1, 2, 2), (2, 2, 2)]
sage: mset = ["s","t","e","i","n"]
sage: Tuples(mset,2).list()
[['s', 's'], ['t', 's'], ['e', 's'], ['i', 's'], ['n', 's'],
['s', 't'], ['t', 't'], ['e', 't'], ['i', 't'], ['n', 't'],
['s', 'e'], ['t', 'e'], ['e', 'e'], ['i', 'e'], ['n', 'e'],
['s', 'i'], ['t', 'i'], ['e', 'i'], ['i', 'i'], ['n', 'i'],
['s', 'n'], ['t', 'n'], ['e', 'n'], ['i', 'n'], ['n', 'n']]
[('s', 's'), ('t', 's'), ('e', 's'), ('i', 's'), ('n', 's'),
('s', 't'), ('t', 't'), ('e', 't'), ('i', 't'), ('n', 't'),
('s', 'e'), ('t', 'e'), ('e', 'e'), ('i', 'e'), ('n', 'e'),
('s', 'i'), ('t', 'i'), ('e', 'i'), ('i', 'i'), ('n', 'i'),
('s', 'n'), ('t', 'n'), ('e', 'n'), ('i', 'n'), ('n', 'n')]

::

sage: K.<a> = GF(4, 'a')
sage: mset = [x for x in K if x != 0]
sage: Tuples(mset,2).list()
[[a, a], [a + 1, a], [1, a], [a, a + 1], [a + 1, a + 1], [1, a + 1],
[a, 1], [a + 1, 1], [1, 1]]
[(a, a), (a + 1, a), (1, a), (a, a + 1), (a + 1, a + 1), (1, a + 1),
(a, 1), (a + 1, 1), (1, 1)]
"""
@staticmethod
def __classcall_private__(cls, S, k):
Expand All @@ -75,7 +75,7 @@ def __init__(self, S, k):
"""
self.S = S
self.k = k
self._index_list = [S.index(s) for s in S]
self._index_list = list(dict.fromkeys(S.index(s) for s in S))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not simply using self._index_list = list(range(len(S))) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

may be self._index_list = range(len(S)) is enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to remove duplicates from the list (that is the bug noted in the conversation in the bug report). The reason that I use the dict.fromkeys dance instead of set is so that the order of elements is not changed, leaving the order of the generated tuples the same as before. (The same is true of the seemingly gratuitous reverse in the __iter__)

The reason for the _index_list is to remove duplicates from a list that has non-hashable elements.

range(len(S)) absolutely won't work.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Thanks for the precise answer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is very bad to rely on the dict order behavior (which could change without notice) and it is not clear that this is what the code is trying to do. IMO it would be much better to then run sorted(set(self._index_list)) to get the unique elements.

Copy link
Collaborator

@tscrim tscrim Jun 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

They are just ints. Please read what I actually wrote more carefully.

Edit: Perhaps the fact that I was basing everything off the code before the change was unclear; so self._index_list = [S.index(s) for s in S].

Copy link
Collaborator

Choose a reason for hiding this comment

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

I know the order is currently guaranteed, but that is basically an implementation detail that could change (as it is not the fundamental programming model for a dict). It also makes for very brittle code a change the insertion order means everything gets broken.

Copy link
Contributor

Choose a reason for hiding this comment

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

The order is guaranteed precisely so that one can rely on it in code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Even if there is now a strong guarantee that it won't change (which I would not say there is considering Python has shown it is okay breaking backwards incompatibility), it still is brittle and obfuscated code. Having an explicit sorted makes the intent of the code clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

considering Python has shown it is okay breaking backwards incompatibility

If you're referring to the Python 2 -> Python 3 transition, well that won't happen again.

category = FiniteEnumeratedSets()
Parent.__init__(self, category=category)

Expand All @@ -94,33 +94,21 @@ def __iter__(self):

sage: S = [1,2]
sage: Tuples(S,3).list()
[[1, 1, 1], [2, 1, 1], [1, 2, 1], [2, 2, 1], [1, 1, 2],
[2, 1, 2], [1, 2, 2], [2, 2, 2]]
[(1, 1, 1), (2, 1, 1), (1, 2, 1), (2, 2, 1), (1, 1, 2),
(2, 1, 2), (1, 2, 2), (2, 2, 2)]
sage: mset = ["s","t","e","i","n"]
sage: Tuples(mset,2).list()
[['s', 's'], ['t', 's'], ['e', 's'], ['i', 's'], ['n', 's'],
['s', 't'], ['t', 't'], ['e', 't'], ['i', 't'],
['n', 't'], ['s', 'e'], ['t', 'e'], ['e', 'e'], ['i', 'e'],
['n', 'e'], ['s', 'i'], ['t', 'i'], ['e', 'i'],
['i', 'i'], ['n', 'i'], ['s', 'n'], ['t', 'n'], ['e', 'n'],
['i', 'n'], ['n', 'n']]
"""
S = self.S
k = self.k
import copy
if k <= 0:
yield []
return
if k == 1:
for x in S:
yield [x]
return

for s in S:
for x in Tuples(S, k - 1):
y = copy.copy(x)
y.append(s)
yield y
[('s', 's'), ('t', 's'), ('e', 's'), ('i', 's'), ('n', 's'),
('s', 't'), ('t', 't'), ('e', 't'), ('i', 't'), ('n', 't'),
('s', 'e'), ('t', 'e'), ('e', 'e'), ('i', 'e'), ('n', 'e'),
('s', 'i'), ('t', 'i'), ('e', 'i'), ('i', 'i'), ('n', 'i'),
('s', 'n'), ('t', 'n'), ('e', 'n'), ('i', 'n'), ('n', 'n')]
sage: Tuples((1,1,2),3).list()
[(1, 1, 1), (2, 1, 1), (1, 2, 1), (2, 2, 1), (1, 1, 2),
(2, 1, 2), (1, 2, 2), (2, 2, 2)]
"""
for p in product(self._index_list, repeat=self.k):
yield tuple(self.S[i] for i in reversed(p))

def cardinality(self):
"""
Expand All @@ -133,7 +121,7 @@ def cardinality(self):
sage: Tuples(S,2).cardinality()
25
"""
return ZZ(libgap.NrTuples(self._index_list, ZZ(self.k)))
return ZZ(len(self._index_list)).__pow__(self.k)


Tuples_sk = Tuples
Expand All @@ -151,10 +139,10 @@ class UnorderedTuples(Parent, UniqueRepresentation):

sage: S = [1,2]
sage: UnorderedTuples(S,3).list()
[[1, 1, 1], [1, 1, 2], [1, 2, 2], [2, 2, 2]]
[(1, 1, 1), (1, 1, 2), (1, 2, 2), (2, 2, 2)]
sage: UnorderedTuples(["a","b","c"],2).list()
[['a', 'a'], ['a', 'b'], ['a', 'c'], ['b', 'b'], ['b', 'c'],
['c', 'c']]
[('a', 'a'), ('a', 'b'), ('a', 'c'), ('b', 'b'), ('b', 'c'),
('c', 'c')]
"""
@staticmethod
def __classcall_private__(cls, S, k):
Expand All @@ -178,7 +166,7 @@ def __init__(self, S, k):
"""
self.S = S
self.k = k
self._index_list = [S.index(s) for s in S]
self._index_list = list(dict.fromkeys(S.index(s) for s in S))
Copy link
Contributor

Choose a reason for hiding this comment

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

same here.

category = FiniteEnumeratedSets()
Parent.__init__(self, category=category)

Expand All @@ -191,19 +179,21 @@ def __repr__(self):
"""
return "Unordered tuples of %s of length %s" % (self.S, self.k)

def list(self):
def __iter__(self):
"""
EXAMPLES::

sage: S = [1,2]
sage: UnorderedTuples(S,3).list()
[[1, 1, 1], [1, 1, 2], [1, 2, 2], [2, 2, 2]]
[(1, 1, 1), (1, 1, 2), (1, 2, 2), (2, 2, 2)]
sage: UnorderedTuples(["a","b","c"],2).list()
[['a', 'a'], ['a', 'b'], ['a', 'c'], ['b', 'b'], ['b', 'c'],
['c', 'c']]
[('a', 'a'), ('a', 'b'), ('a', 'c'), ('b', 'b'), ('b', 'c'),
('c', 'c')]
sage: UnorderedTuples([1,1,2],3).list()
[(1, 1, 1), (1, 1, 2), (1, 2, 2), (2, 2, 2)]
"""
ans = libgap.UnorderedTuples(self._index_list, ZZ(self.k))
return [[self.S[i] for i in l] for l in ans]
for ans in combinations_with_replacement(self._index_list, self.k):
yield tuple(self.S[i] for i in ans)

def cardinality(self):
"""
Expand All @@ -213,7 +203,7 @@ def cardinality(self):
sage: UnorderedTuples(S,2).cardinality()
15
"""
return ZZ(libgap.NrUnorderedTuples(self._index_list, ZZ(self.k)))
return binomial(len(self._index_list) + self.k - 1, self.k)


UnorderedTuples_sk = UnorderedTuples
4 changes: 2 additions & 2 deletions src/sage/schemes/projective/projective_space.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2068,7 +2068,7 @@ def subscheme_from_Chow_form(self, Ch, dim):
L1 = []
for t in UnorderedTuples(list(range(n + 1)), dim + 1):
if all(t[i] < t[i + 1] for i in range(dim)):
L1.append(t)
L1.append(list(t))
# create the dual brackets
L2 = []
signs = []
Expand Down Expand Up @@ -2374,7 +2374,7 @@ def rational_points(self, bound=0):
for ai in R:
P[i] = ai
for tup in S[i - 1]:
if gcd([ai] + tup) == 1:
if gcd([ai] + list(tup)) == 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, gcd((ai,) + tup) is more appropriate.

sage: def A(a, tup):
....:     return [a] + list(tup)
....: 
sage: def B(a, tup):
....:     return (a,) + tup
....: 
sage: tup = tuple(range(10))
sage: %timeit A(100, tup)
134 ns ± 0.0589 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)
sage: %timeit B(100, tup)
84.8 ns ± 0.0729 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)
sage: tup = tuple(range(4))
sage: %timeit A(100, tup)
123 ns ± 1.49 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)
sage: %timeit B(100, tup)
79.1 ns ± 0.0314 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)
sage: %timeit gcd(A(100, tup))
7.03 µs ± 4.32 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
sage: %timeit gcd(B(100, tup))
6.95 µs ± 11.7 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

Copy link
Collaborator

@tscrim tscrim Jun 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That's not a fair comparison because of the extra (unnecessary) list cast, which creates a copy. (Recall the old code was returning a list.)

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not a fair comparison because of the extra (unnecessary) list cast, which creates a copy. (Recall the old code was returning a list.)
I agree. Since the code now returns a tuple, it's better to use (ai,) + tup than casting to list.

for j in range(i):
P[j] = tup[j]
pts.append(self(P))
Expand Down