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.

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

Fix enumeration/cardinality of UnorderedTuples, remove dependency on GAP (for modularization) #35784

Closed
mkoeppe opened this issue Jun 16, 2023 · 3 comments · Fixed by #35812
Closed

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Jun 16, 2023

That's a gratuitous dependency on a heavy library, which could be rewritten using just itertools.

Part of:

@deinst
Copy link
Contributor

deinst commented Jun 21, 2023

I am more than willing to fix this, but I am less than certain what should be done in the case where the underlying iterable has duplicates. For example,

sage: S=[1,1,2,3,4,5]
sage: t = Tuples(S,2)
sage: t.cardinality()
25
sage: len(t.list())
36

My instinct is that Tuples.__iter__() is incorrectly implemented (it should ignore the second '1'), but I don't want to break anyone that might depend on the original behavior.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 21, 2023

This does look like a bug to me. @tscrim, @fchapoton, do you agree?

@tscrim
Copy link
Collaborator

tscrim commented Jun 22, 2023

I agree that it is a bug. As such, @deinst you do not have to fix other people's code relying on bad behavior. I would just implement both iterators with itertools and directly implement the formulas for their cardinality() methods.

@mkoeppe mkoeppe changed the title Remove dependency on GAP for enumerating UnorderedTuples (modularization) Fix enumeration/cardinality of UnorderedTuples, remove dependency on GAP (for modularization) Jun 22, 2023
deinst added a commit to deinst/sage that referenced this issue Jun 22, 2023
This commit is just a cosmetic change from the last (deleting a
commented out line).  This should finish bug sagemath#35784.
@deinst deinst mentioned this issue Jun 22, 2023
5 tasks
@vbraun vbraun closed this as completed in 9119b4d Jul 9, 2023
@mkoeppe mkoeppe added this to the sage-10.1 milestone Jul 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants