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

use libgap for abelian subgroups #34308

Closed
fchapoton opened this issue Aug 8, 2022 · 9 comments
Closed

use libgap for abelian subgroups #34308

fchapoton opened this issue Aug 8, 2022 · 9 comments

Comments

@fchapoton
Copy link
Contributor

instead of gap pexpect

CC: @tscrim @kwankyu @slel

Component: group theory

Author: Frédéric Chapoton

Branch/Commit: da3bc66

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/34308

@fchapoton fchapoton added this to the sage-9.7 milestone Aug 8, 2022
@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/34308

@fchapoton
Copy link
Contributor Author

Commit: da3bc66

@fchapoton
Copy link
Contributor Author

New commits:

da3bc66refactor abelian subgroup with libgap

@fchapoton
Copy link
Contributor Author

comment:2

green bot, so please review

@tscrim
Copy link
Collaborator

tscrim commented Aug 9, 2022

comment:3

I recall that len([1 for g in gen]) is faster than sum(1 for g in gen), but it is not as efficient in memory. Is this why you went this route? Do we expect these lists to all be small? I vaguely recall there is a Python count function/method; could that be faster?

@fchapoton
Copy link
Contributor Author

comment:4

hello Travis,

I am just trying to get rid of the gap pexpect interface here. I am surprised that I finally managed to do that (it must be my third or fourth tentative).

I am not sure it is worth optimising further. The whole abelian group setup is IMHO a complete mess, that would deserve a complete rewriting.

@tscrim
Copy link
Collaborator

tscrim commented Aug 9, 2022

comment:5

Of course. I was just wondering if there was something specific behind this choice.

@tscrim
Copy link
Collaborator

tscrim commented Aug 9, 2022

Reviewer: Travis Scrimshaw

@vbraun
Copy link
Member

vbraun commented Aug 30, 2022

Changed branch from u/chapoton/34308 to da3bc66

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants