SmallPermutationGroups #35225

dwbump · 2023-03-02T14:12:34Z

📚 Description

GAP has a database of Small Groups identified by a pair of numbers [n,k] where n is the order and k is an integer used to differentiate different groups of the same order. Thus k is bounded by oeis(1)[n], the number of isomorphism classes of groups of order n. Since Sage PermutationGroups have many useful methods, it is good to have PermutationGroup realizations of GAP SmallGroups. The docstring verifies that the new SmallPermutationGroup class is able to construct all groups of order <= 64, though of course groups of order >64 are also available.

📝 Checklist

[ x] I have made sure that the title is self-explanatory and the description concisely explains the PR.
I have linked an issue or discussion.
[ x] I have created tests covering the changes.
[ x] I have updated the documentation accordingly.

⌛ Dependencies

dimpase

Can you use libgap rather than gap? I.e.

self._gap_small_group = libgap.SmallGroup(order,gap_id)

dimpase · 2023-03-02T15:41:35Z

src/sage/groups/perm_gps/permgroup_named.py

@@ -87,7 +89,9 @@
 from pathlib import Path

 from sage.rings.all import Integer
+from sage.interfaces.gap import gap


please no more dependencies on pexpect gap. We are trying to get rid of it in #26902

If we do the suggested self._gap_small_group = libgap.SmallGroup(order,gap_id) then we lose some functionality, needed for an application (FusionDouble, probably an upcoming PR). We need the methods conjugacy_classes_representatives and centralizer.

I haven't figured out why these do not work when we use libgap. I tried making SmallPermutationGroup inherit from PermutationGroup_generic since that class has the needed methods.

is FusionDouble a new functionality, not in Sage yet?

Yes, analogous to FusionRing one can construct the Fusion Rings of the Drinfeld Double of a finite group.

codecov-commenter · 2023-03-02T17:29:30Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.02 ⚠️

Comparison is base (f449b14) 88.62% compared to head (83e3095) 88.61%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #35225      +/-   ##
===========================================
- Coverage    88.62%   88.61%   -0.02%     
===========================================
  Files         2148     2148              
  Lines       398653   398868     +215     
===========================================
+ Hits        353308   353450     +142     
- Misses       45345    45418      +73

Impacted Files	Coverage Δ
src/sage/groups/perm_gps/all.py	`100.00% <ø> (ø)`
src/sage/groups/perm_gps/permgroup_named.py	`92.60% <100.00%> (+0.16%)`	⬆️

... and 126 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

dimpase · 2023-03-02T18:00:45Z

is this code available somewhere? I can try to port it to libgap.

dwbump · 2023-03-02T18:32:14Z

is this code available somewhere? I can try to port it to libgap.

Working code exists (written by Wenqi Li and myself) but work needs to be done before it is ready for review. Particularly, I think Willie Aboumrad and Travis Scrimshaw were going to try to refactor it into the existing FusionRing class.

I think what is needed now is an understanding as to why changing gap to libgap here breaks the centralizer and conjugacy_class_representatives methods.

dimpase · 2023-03-02T18:40:57Z

On Thu, 2 Mar 2023, 19:32 Daniel Bump, ***@***.***> wrote: is this code available somewhere? I can try to port it to libgap. Working code exists (written by Wenqi Li and myself) but work needs to be done before it is ready for review. Particularly, I think Willie Aboumrad and Travis Scrimshaw were going to try to refactor it into the existing FusionRing class. I think what is needed now is an understanding as to why changing gap to libgap here breaks the centralizer and conjugacy_class_representatives methods.

it could be that the latter need porting to libgap. Can you point at more precise code location? —

…

Reply to this email directly, view it on GitHub <#35225 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJXYHHQ3U24CQ5P67OX76DW2DRTRANCNFSM6AAAAAAVNN2MVE> . You are receiving this because your review was requested.Message ID: ***@***.***>

dwbump · 2023-03-02T19:20:27Z

it could be that the latter need porting to libgap.
Can you point at more precise code location?

dwbump · 2023-03-02T19:27:14Z

it could be that the latter need porting to libgap.
Can you point at more precise code location?

I added a doctest at line 3479 of the file, to compute some centralizers. So if you uncomment line 3487, changing gap to libgap, that breaks the doctest. The centralizer and conjugacy_classes_representatives methods are implemented in the PermutationGroup_generic class, in permgroup.py.

dimpase · 2023-03-02T20:16:55Z

thanks - I'll look at it after the conference in Luminy I'm attending ends, tomorrow.

ChrisJefferson · 2023-03-03T03:40:34Z

Some issues with this PR:

It might be worth mentioning in the docs that this group isn't fixed -- while it will always be isomorphic to the SmallGroup in question, it can change over time. Also, in many cases [n,k] will be a permutation group on n points -- but that won't always be true.

Also, there is a practical issue. Image(IsomorphismPermGroup(g)) doesn't return what you want (always) -- as it can return a larger group (this behaviour may be changed in a future version of GAP to what you want, but it isn't that right now -- gap-system/gap#5355 ).

Instead you want to call Image(IsomorphismPermGroup(g),g)

@ChrisJefferson

as explained by @ChrisJefferson: Image(IsomorphismPermGroup(g)) doesn't return what you want (always) -- as it can return a larger group (this behaviour may be changed in a future version of GAP to what you want, but it isn't that right now -- gap-system/gap#5355 ). Instead you want to call Image(IsomorphismPermGroup(g),g)

dimpase · 2023-03-03T11:25:16Z

Instead you want to call Image(IsomorphismPermGroup(g),g)

fixed by a57a9c0

dwbump · 2023-03-03T14:31:46Z

It might be worth mentioning in the docs that this group isn't fixed -- while it will always be isomorphic to the SmallGroup in question, it can change over time. Also, in many cases [n,k] will be a permutation group on n points -- but that won't always be true.

I edited the docstring to mention these points. The other comment on Image was addressed by a commit by dimpase.

dimpase · 2023-03-03T14:34:33Z

the immediate libgap issue is that the group in the docstring is not converted to a permutation group, it's a just a GAP integer 6. Is it related to in many cases [n,k] will be a permutation group on n points -- but that won't always be true. ?

dwbump · 2023-03-03T16:20:21Z

the immediate libgap issue is that the group in the docstring is not converted to a permutation group, it's a just a GAP integer 6. Is it related to in many cases [n,k] will be a permutation group on n points -- but that won't always be true. ?

It doesn't seem related to this point. I say this because for some k the group [12,k] is of degree 12 and for other k it is of degree 7, but in every case the libgap version of the group fails this test.

dimpase · 2023-03-03T16:43:13Z

On Fri, Mar 3, 2023 at 5:20 PM Daniel Bump ***@***.***> wrote: the immediate libgap issue is that the group in the docstring is not converted to a permutation group, it's a just a GAP integer 6. Is it related to in many cases [n,k] will be a permutation group on n points -- but that won't always be true. ? It doesn't seem related to this point. I say this because for some k the group [12,k] is of degree 12 and for other k it is of degree 7, but in every case the libgap version of the group fails this test.

I don't understand how the gap version works. Ineed, you pass the number of moved points to the constructor of PermutationGroup_generic, but its signature is def __init__(self, gens=None, gap_group=None, canonicalize=True, domain=None, category=None): and in the code I don't see whether there is any provision for handling this number of moved points. Could you please explain what you're trying to do here. As well, we better pass gap_group, not gens. And it would help if you used explicit parameter names...

…

— Reply to this email directly, view it on GitHub <#35225 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJXYHAXERIHSFZAHZKRV2TW2IK5BANCNFSM6AAAAAAVNN2MVE> . You are receiving this because your review was requested.Message ID: ***@***.***>

dwbump · 2023-03-03T17:01:56Z

Ineed, you pass the number of moved points to the constructor of PermutationGroup_generic, but its signature is
def init(self, gens=None, gap_group=None, canonicalize=True, domain=None, category=None):

You are right, for the libgap group we do not want to pass the degree of the permutation representation, we want to pass the gap group. This seems to be the key to fixing the libgap group, which I'll have to take care of later but thanks for noticing this.

tscrim · 2023-03-16T14:25:38Z

@dimpase @fingolfin Is this a positive review or not? It seems like the PR is approved, but the status badge wasn't changed. Also, what about #35225 (comment)?

dimpase · 2023-03-16T14:41:18Z

I'd like to hear about your opinion on #35225 (comment) - why do you recompute known group orders?

dimpase · 2023-03-16T14:42:10Z

that's why I kept the tag "needs review" on.

tscrim · 2023-03-16T15:04:10Z

I'd like to hear about your opinion on #35225 (comment) - why do you recompute known group orders?

Which "you"? Me or @dwbump?

dimpase · 2023-03-16T15:14:48Z

Well, someone who knows the code involved.

tscrim · 2023-03-16T22:46:07Z

So the current implementation uses what is available by default in Sage for permutation groups. In particular, it does eventually call GAP's Size() whenever the group is not generated by transpositions, but it checks this.

Now the order is passed to the constructor within Sage, but @dwbump has done is just store that as an attribute, which I said was okay as writing a custom order() method to just return said attribute introduces some added complexity of the code. In particular, it does the "computation" I just mentioned, which essentially leaves it up to GAP. Although I guess the underlying GAP group is actually the isomorphic permutation group, so perhaps GAP is having to do a bit more computation, but the groups are "small" so this should be quick.

Well...I would push the change overriding the order() method to my own branch based on @dwbump's branch, but I really don't want to do a whole new PR just for this. It seems less bad to do a PR based on this one, albeit still somewhat fragmented and should be merged at the same time...

…ing it.

…o small-permutation-groups

dwbump · 2023-03-20T17:19:34Z

I added an order method that just returns self._n.

So the current implementation uses what is available by default in Sage for permutation groups. In particular, it does eventually call GAP's Size() whenever the group is not generated by transpositions, but it checks this.

github-actions · 2023-03-20T19:14:26Z

Documentation preview for this PR is ready! 🎉
Built with commit: 83e3095

tscrim · 2023-03-21T14:43:19Z

Thank you. I think this takes care of the last comments by @dimpase. Are there any objections to setting a positive review?

dimpase · 2023-03-21T17:31:00Z

lgtm

vbraun · 2023-03-30T22:48:03Z

Tries to download stuff from the internet:

      File "/var/lib/buildbot/slave/sage_git/build/src/sage/databases/oeis.py", line 1289, in first_terms
        return to_tuple(" ".join(flatten([self._field(a) for a in fields])))[:number]
                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/var/lib/buildbot/slave/sage_git/build/src/sage/databases/oeis.py", line 1289, in <listcomp>
        return to_tuple(" ".join(flatten([self._field(a) for a in fields])))[:number]
                                          ^^^^^^^^^^^^^^
      File "/var/lib/buildbot/slave/sage_git/build/src/sage/databases/oeis.py", line 752, in _field
        for line in self.raw_entry().splitlines():
                    ^^^^^^^^^^^^^^^^
      File "/var/lib/buildbot/slave/sage_git/build/src/sage/databases/oeis.py", line 860, in raw_entry
        self.online_update()
      File "/var/lib/buildbot/slave/sage_git/build/src/sage/databases/oeis.py", line 730, in online_update
        self._raw = _fetch(url).split('\n\n')[2]
                    ^^^^^^^^^^^
      File "/var/lib/buildbot/slave/sage_git/build/src/sage/databases/oeis.py", line 205, in _fetch
        raise IOError("%s\nerror fetching %s" % (msg, url))
    OSError: <urlopen error [Errno 110] Connection timed out>
    error fetching https://oeis.org/search?q=A000001&n=1&fmt=text
**********************************************************************
1 item had failures:
   1 of   8 in sage.groups.perm_gps.permgroup_named.SmallPermutationGroup
    [539 tests, 1 failure, 133.62 s]
----------------------------------------------------------------------
sage -t --long --random-seed=0 src/sage/groups/perm_gps/permgroup_named.py  # 1 doctest failed
----------------------------------------------------------------------

http://build.sagemath.org/#/builders/6/builds/4

tscrim · 2023-03-31T00:40:56Z

src/sage/groups/perm_gps/permgroup_named.py

+        [ 1  1 -1  1 -1 -1]
+        [ 2  0 -2 -1  0  1]
+        [ 2  0  2 -1  0 -1]
+        sage: all(SmallPermutationGroup(n,k).id()==[n,k] for n in [1..64] for k in [1..oeis(1)[n]])


This test needs to be marked with # optional - internet and/or replaced by something that is manually inputted. (I would at least include part of the latter since it means it will get more regularly tested.)

Why does it query data from OEIS that is part of the very small groups library that is eing wrapped here in the first place?

I replaced the oeis(1) by a call to libgap.NumberSmallGroups.

dwbump · 2023-04-02T23:58:25Z

Volker removed the Positive Review flair because of the oeis(1) call. I fixed that, so I added Needs Review. I am not sure why the pdf documentation is failing to build.

tscrim · 2023-04-03T03:27:13Z

LGTM modulo one trivial whitespace thing:

-sage: def numgps(n) : return ZZ(libgap.NumberSmallGroups(n))
+sage: def numgps(n : return ZZ(libgap.NumberSmallGroups(n))

I’m not sure why the pdf docs build is failing either. Did you check locally?

dwbump · 2023-04-03T14:42:18Z

LGTM modulo one trivial whitespace thing:

-sage: def numgps(n) : return ZZ(libgap.NumberSmallGroups(n))
+sage: def numgps(n : return ZZ(libgap.NumberSmallGroups(n))

I think you meant to delete the space before the colon and instead deleted the paren.

I’m not sure why the pdf docs build is failing either. Did you check locally?

On my machine, make doc-pdf runs to completion and does not fail.

tscrim

Yes, that's right. Thank you.

SmallPermutationGroups

0fc0bc5

dwbump requested review from tscrim and dimpase March 2, 2023 14:14

dwbump added t: enhancement s: needs review c: group theory labels Mar 2, 2023

dimpase reviewed Mar 2, 2023

View reviewed changes

dimpase suggested changes Mar 2, 2023

View reviewed changes

added doctest that is broken if we change to libgap

b8eacec

dwbump closed this Mar 2, 2023

dwbump reopened this Mar 2, 2023

dwbmscz added 2 commits March 3, 2023 10:57

SmallPermutationGroups

353abd4

added doctest that is broken if we change to libgap

a1a3084

dimpase force-pushed the small-permutation-groups branch from b8eacec to a1a3084 Compare March 3, 2023 10:57

Remark on generators and degree in SmallPermutationGroup docstring

32bc68a

Merge branch 'develop' into small-permutation-groups

b5c2104

dimpase added s: positive review and removed s: needs review labels Mar 17, 2023

dwbmscz added 2 commits March 20, 2023 10:11

SmallPermutationGroup returns self._n as its order, to avoid recomput…

d4a657e

…ing it.

Merge branch 'small-permutation-groups' of github.com:dwbump/sage int…

83e3095

…o small-permutation-groups

vbraun removed the s: positive review label Mar 30, 2023

tscrim requested changes Mar 31, 2023

View reviewed changes

replace oeis(1) by call to libgap.NumberSmallGroups

526cb5d

dwbump added the s: needs review label Apr 2, 2023

whitespace in def numgps (permgroup_named.py)

03a1839

tscrim approved these changes Apr 3, 2023

View reviewed changes

tscrim added s: positive review and removed s: needs review labels Apr 3, 2023

vbraun merged commit fdfe2c2 into sagemath:develop Apr 6, 2023
3 of 5 checks passed

mkoeppe removed the s: positive review label Apr 7, 2023

mkoeppe added this to the sage-10.0 milestone Apr 7, 2023

SmallPermutationGroups #35225

SmallPermutationGroups #35225

Conversation

dwbump commented Mar 2, 2023

📚 Description

📝 Checklist

⌛ Dependencies

dimpase left a comment

Choose a reason for hiding this comment

dimpase Mar 2, 2023

Choose a reason for hiding this comment

dwbump Mar 2, 2023

Choose a reason for hiding this comment

dimpase Mar 2, 2023

Choose a reason for hiding this comment

dwbump Mar 2, 2023

Choose a reason for hiding this comment

codecov-commenter commented Mar 2, 2023 • edited

Codecov Report

dimpase commented Mar 2, 2023

dwbump commented Mar 2, 2023

dimpase commented Mar 2, 2023 via email

dwbump commented Mar 2, 2023

dwbump commented Mar 2, 2023

dimpase commented Mar 2, 2023

ChrisJefferson commented Mar 3, 2023

dimpase commented Mar 3, 2023

dwbump commented Mar 3, 2023

dimpase commented Mar 3, 2023

dwbump commented Mar 3, 2023

dimpase commented Mar 3, 2023 via email

dwbump commented Mar 3, 2023

tscrim commented Mar 16, 2023

dimpase commented Mar 16, 2023

dimpase commented Mar 16, 2023

tscrim commented Mar 16, 2023

dimpase commented Mar 16, 2023

tscrim commented Mar 16, 2023

dwbump commented Mar 20, 2023

github-actions bot commented Mar 20, 2023

tscrim commented Mar 21, 2023

dimpase commented Mar 21, 2023

vbraun commented Mar 30, 2023 • edited

tscrim Mar 31, 2023

Choose a reason for hiding this comment

fingolfin Mar 31, 2023 • edited

Choose a reason for hiding this comment

dwbump Apr 1, 2023

Choose a reason for hiding this comment

dwbump commented Apr 2, 2023

tscrim commented Apr 3, 2023

dwbump commented Apr 3, 2023

tscrim left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 2, 2023 •

edited

vbraun commented Mar 30, 2023 •

edited

fingolfin Mar 31, 2023 •

edited