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

Decouple tuple #35812

Merged
merged 8 commits into from
Jul 9, 2023
Merged

Decouple tuple #35812

Changes from 3 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
41 changes: 16 additions & 25 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 Down Expand Up @@ -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 Down Expand Up @@ -104,23 +104,12 @@ def __iter__(self):
['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]]
"""
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
for p in product(self._index_list, repeat=self.k):
yield [self.S[i] for i in reversed(p)]

def cardinality(self):
"""
Expand All @@ -133,7 +122,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 Down Expand Up @@ -178,7 +167,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,7 +180,7 @@ def __repr__(self):
"""
return "Unordered tuples of %s of length %s" % (self.S, self.k)

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

Expand All @@ -201,9 +190,11 @@ def list(self):
sage: UnorderedTuples(["a","b","c"],2).list()
[['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 [self.S[i] for i in ans]

def cardinality(self):
"""
Expand All @@ -213,7 +204,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