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

Iteration over infinite abelian groups #38961

Merged
merged 3 commits into from
Nov 16, 2024

Conversation

DaveWitteMorris
Copy link
Member

Fixes #30751.

This PR adds code to the __iter__ method of AbelianGroup to handle infinite groups. For backward compatibility (and because the finite case is simpler and probably more efficient), the code for the finite case has been retained.

📝 Checklist

  • The title is concise and informative.
  • 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 and checked the documentation preview.

Copy link

github-actions bot commented Nov 13, 2024

Documentation preview for this PR (built with commit 4c5a738; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

Comment on lines 1315 to 1324
if 0 not in invs:
# The group is finite
for t in mrange(invs):
yield self(t)
else:
# A similar approach works for infinite groups.
# (This would also work for finite groups, but is more complicated.)
from sage.misc.mrange import cantor_product
for t in cantor_product(*[range(n) if n > 0 else ZZ for n in invs]):
yield self(t)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this can be simplified slightly:

Suggested change
if 0 not in invs:
# The group is finite
for t in mrange(invs):
yield self(t)
else:
# A similar approach works for infinite groups.
# (This would also work for finite groups, but is more complicated.)
from sage.misc.mrange import cantor_product
for t in cantor_product(*[range(n) if n > 0 else ZZ for n in invs]):
yield self(t)
if 0 not in invs:
# The group is finite
yield from map(self, mrange(invs))
else:
# A similar approach works for infinite groups.
# (This would also work for finite groups, but is more complicated.)
from sage.misc.mrange import cantor_product
yield from map(self, cantor_product(*[range(n) if n else ZZ for n in invs]))

I suppose that negative orders should not be allowed, although:

sage: G = AbelianGroup([-4])
sage: G.cardinality()
-4

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that your code is an improvement, so I made the changes. (At least, I think I did, but I don't really understand github.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened issue #38967 to correct the problem with negative orders.

Copy link
Contributor

@mantepse mantepse left a comment

Choose a reason for hiding this comment

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

Perfekt!

@DaveWitteMorris
Copy link
Member Author

Thanks for the review and your comments!

@vbraun vbraun merged commit c007973 into sagemath:develop Nov 16, 2024
24 of 25 checks passed
@DaveWitteMorris DaveWitteMorris deleted the 30751iteration branch December 8, 2024 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Iteration over infinite abelian groups
3 participants