Decouple tuple #35812

deinst · 2023-06-22T22:03:03Z

Change sage/combinat/tuple.py to use itertools instead of GAP.

📚 Description

This fixes #35784.
Iterating Tuples delegates to itertools.product
Iterating UnorderedTuples delegates to itertools.combinations_with_replacement

📝 Checklist

The title is concise, informative, and self-explanatory.
The description explains in detail what this PR is about.
I have linked a relevant issue or discussion.
I have created tests covering the changes.
I have updated the documentation accordingly.

⌛ Dependencies

The generators for the classes Tuples and UnorderedTuples used the GAP for generating Tuples. This could be done much more simply using itertools. Also fixed a bug where if the underlying list had multiple identical elements then redundant tuples were generated.

This commit is just a cosmetic change from the last (deleting a commented out line). This should finish bug sagemath#35784.

mkoeppe · 2023-06-23T00:01:56Z

Could you add a test that illustrates the fixed bug?

Also removed a blank line that irritated the linter.

deinst · 2023-06-23T02:14:20Z

This adds checks having duplicates in the underlying lists. It also removed a blank line that annoyed the linter

dcoudert · 2023-06-24T12:16:13Z

src/sage/combinat/tuple.py

@@ -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))


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

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

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.

I see. Thanks for the precise answer.

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.

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].

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.

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

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.

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.

dcoudert · 2023-06-24T12:16:50Z

src/sage/combinat/tuple.py

@@ -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))


dcoudert · 2023-06-24T16:17:46Z

It is surprising to have Tuples and UnorderedTuples yielding lists and not tuples, but it might be a significant amount of work to change.

deinst · 2023-06-25T00:56:06Z

It is no problem to have it return tuples. The time consuming part is rerunning all the tests. I'll update the pull request as soon as the testing is done (assuming nothing that uses the module breaks).

deinst · 2023-06-25T01:36:45Z

schemes/projective/projective_space.py expects Tuples to return a list. This appears to be the only place that this assumption is made, and it appears easy to fix (just convert each tuple to a list as we use it), though I am reluctant to poke my fingers in code that I do not completely understand.

dcoudert · 2023-06-25T08:06:20Z

I had a look at schemes/projective/projective_space.py and the only required modification is:

-                    if gcd([ai] + tup) == 1:   # when Tuples returns lists
+                    if gcd((ai,) + tup) == 1:  # when Tuples returns tuples

However, it's best to make a specific issue / PR if we change the return type from list to tuple. We can put this in the wishlist for the moment.

dcoudert

LGTM.

This has Tuples and UnorderedTuples return tuples. Need to fix up schemes/projective/projective_space.py as it assumes the tuples are lists.

dcoudert · 2023-06-25T13:27:40Z

src/sage/schemes/projective/projective_space.py

@@ -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:


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)

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.)

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.

Part of the ongoing project to free Tuple of GAP.

dcoudert

LGTM.

The use of the python dict guarantee of insertion order of keys was confusing. There was no compelling reason (other than an aversion to change) for keeping insertion order.

mkoeppe · 2023-06-29T17:12:16Z

Is there a remaining work item, or is it ready for review again?

deinst · 2023-06-29T17:20:33Z

It should be good to go.

tscrim

LGTM. Thanks.

vbraun · 2023-07-02T15:11:41Z

Merge conflict

SageMath version 10.1.beta5, Release Date: 2023-07-01

github-actions · 2023-07-02T22:31:55Z

Documentation preview for this PR (built with commit f79c0d0; changes) is ready! 🎉

deinst · 2023-07-02T23:11:05Z

Thanks @mkoeppe . Just for future reference, is there some way I could have foreseen this and prevented everyone extra work?

mkoeppe · 2023-07-03T00:13:32Z

Not really; I'm doing bulk changes at the moment, which often create (easy to resolve) conflicts.

dcoudert

LGTM.

deinst added 2 commits June 21, 2023 23:00

Mind the GAP

e057b27

The generators for the classes Tuples and UnorderedTuples used the GAP for generating Tuples. This could be done much more simply using itertools. Also fixed a bug where if the underlying list had multiple identical elements then redundant tuples were generated.

Finish decoupling tuple.py from GAP.

aba3954

This commit is just a cosmetic change from the last (deleting a commented out line). This should finish bug sagemath#35784.

mkoeppe added t: refactoring c: combinatorics labels Jun 23, 2023

Added testcases to Tuples for undelying lists with duplicates

5702830

Also removed a blank line that irritated the linter.

mkoeppe requested a review from tscrim June 23, 2023 02:50

mkoeppe added the s: needs review label Jun 23, 2023

dcoudert suggested changes Jun 24, 2023

View reviewed changes

dcoudert approved these changes Jun 25, 2023

View reviewed changes

dcoudert added s: positive review and removed s: needs review labels Jun 25, 2023

Have tuple.py return tuples instead of lists

dffe683

This has Tuples and UnorderedTuples return tuples. Need to fix up schemes/projective/projective_space.py as it assumes the tuples are lists.

dcoudert suggested changes Jun 25, 2023

View reviewed changes

dcoudert added s: needs review and removed s: positive review labels Jun 25, 2023

Switch tuple to list in projective_space.py

14e7540

Part of the ongoing project to free Tuple of GAP.

dcoudert approved these changes Jun 25, 2023

View reviewed changes

dcoudert added s: positive review and removed s: needs review labels Jun 25, 2023

tscrim added s: needs work and removed s: positive review labels Jun 26, 2023

deinst added 2 commits June 26, 2023 11:33

Merge branch 'develop' into decouple_tuple

f40c739

Change dict to set

660653c

The use of the python dict guarantee of insertion order of keys was confusing. There was no compelling reason (other than an aversion to change) for keeping insertion order.

mkoeppe added s: needs review and removed s: needs work labels Jun 29, 2023

tscrim added s: positive review and removed s: needs review labels Jun 30, 2023

tscrim approved these changes Jun 30, 2023

View reviewed changes

vbraun added the s: needs work label Jul 2, 2023

dcoudert removed the s: positive review label Jul 2, 2023

Merge tag '10.1.beta5' into decouple_tuple

f79c0d0

SageMath version 10.1.beta5, Release Date: 2023-07-01

mkoeppe added s: needs review and removed s: needs work labels Jul 2, 2023

dcoudert approved these changes Jul 3, 2023

View reviewed changes

dcoudert added s: positive review and removed s: needs review labels Jul 3, 2023

vbraun merged commit 9119b4d into sagemath:develop Jul 9, 2023
14 of 15 checks passed

mkoeppe added this to the sage-10.1 milestone Jul 9, 2023

mkoeppe removed the s: positive review label Jul 9, 2023

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple tuple #35812

Decouple tuple #35812

deinst commented Jun 22, 2023

mkoeppe commented Jun 23, 2023

deinst commented Jun 23, 2023

dcoudert Jun 24, 2023

dcoudert Jun 24, 2023

deinst Jun 24, 2023

dcoudert Jun 24, 2023

tscrim Jun 26, 2023

tscrim Jun 27, 2023 •

edited

Loading

tscrim Jun 27, 2023

mkoeppe Jun 28, 2023

tscrim Jun 28, 2023

mkoeppe Jun 29, 2023

dcoudert Jun 24, 2023

dcoudert commented Jun 24, 2023

deinst commented Jun 25, 2023

deinst commented Jun 25, 2023 •

edited

Loading

dcoudert commented Jun 25, 2023

dcoudert left a comment

dcoudert Jun 25, 2023

tscrim Jun 26, 2023 •

edited

Loading

dcoudert Jun 26, 2023

dcoudert left a comment

mkoeppe commented Jun 29, 2023

deinst commented Jun 29, 2023

tscrim left a comment

vbraun commented Jul 2, 2023

github-actions bot commented Jul 2, 2023

deinst commented Jul 2, 2023

mkoeppe commented Jul 3, 2023

dcoudert left a comment

Decouple tuple #35812

Decouple tuple #35812

Conversation

deinst commented Jun 22, 2023

📚 Description

📝 Checklist

⌛ Dependencies

mkoeppe commented Jun 23, 2023

deinst commented Jun 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tscrim Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcoudert commented Jun 24, 2023

deinst commented Jun 25, 2023

deinst commented Jun 25, 2023 • edited Loading

dcoudert commented Jun 25, 2023

dcoudert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tscrim Jun 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcoudert left a comment

Choose a reason for hiding this comment

mkoeppe commented Jun 29, 2023

deinst commented Jun 29, 2023

tscrim left a comment

Choose a reason for hiding this comment

vbraun commented Jul 2, 2023

github-actions bot commented Jul 2, 2023

deinst commented Jul 2, 2023

mkoeppe commented Jul 3, 2023

dcoudert left a comment

Choose a reason for hiding this comment

tscrim Jun 27, 2023 •

edited

Loading

deinst commented Jun 25, 2023 •

edited

Loading

tscrim Jun 26, 2023 •

edited

Loading