Opened 3 years ago
Closed 3 years ago
#24521 closed enhancement (fixed)
Abelian Groups with Gap
Reported by:  sbrandhorst  Owned by:  

Priority:  major  Milestone:  sage8.2 
Component:  group theory  Keywords:  
Cc:  vbraun, tscrim  Merged in:  
Authors:  Simon Brandhorst  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  477b66d (Commits, GitHub, GitLab)  Commit:  477b66d14b8eceb0ab2d91a6aa36be646dd16526 
Dependencies:  Stopgaps: 
Description
Interface our abelian groups with the abelian groups in gap through libgap.
Change History (35)
comment:1 Changed 3 years ago by
 Branch set to u/sbrandhorst/abelian_groups_with_gap
comment:2 Changed 3 years ago by
 Cc tscrim added
 Commit set to f9af766f54b7bf3200c7fe55f7da2304bd56a856
comment:3 Changed 3 years ago by
 Commit changed from f9af766f54b7bf3200c7fe55f7da2304bd56a856 to 0b13ad7861cddf55f6a5de067ef7541f7d2cac52
Branch pushed to git repo; I updated commit sha1. New commits:
0b13ad7  Documentation an bugfixes.

comment:4 Changed 3 years ago by
 Status changed from new to needs_review
comment:5 Changed 3 years ago by
 Status changed from needs_review to needs_work
Some comments:
 A number of your 1line descriptions are missing periods/fullstops at the end.
AbelianGroupElement_gap.__reduce__
is missingEXAMPLES::
orTESTS::
. (Is this method necessary?) Your
INPUT:
andOUTPUT:
blocks are overindented. AbelianGroup_gap.__init__
is missing documentation. I would recommend having theTestSuite
test there and in the classlevel doc having an actual example(s) of how to construct and use the class. the
 ``A``  an `AbelianGroup`
means that "AbelianGroup?" is latex formatting (needs 2 backticks on both sides), but I don't understand why you do not link to the specific class (or construction function) in Sage.  There should not be a
__hash__
function because that should be handled byUniqueRepresentation
. _coerce_map_from_
is missing documentation. I do not understand the
sage
method.  The
OUTPUT:
ofsubgroup
is strange. I understand why you are saying that (most subgroups do not keep track of their ambient group(s)), but it reads very awkwardly. elementary_divisors
does not have any doctests.
comment:6 Changed 3 years ago by
 Commit changed from 0b13ad7861cddf55f6a5de067ef7541f7d2cac52 to 74234d4de9bf109ded91c74c0657678317de7bb3
Branch pushed to git repo; I updated commit sha1. New commits:
74234d4  Documentation and removed hash method for AbelianGroup_gap

comment:7 Changed 3 years ago by
Thank you for your comments.
I guess you are right the .sage method makes little sense. I will replace it by a coercion to the sage implementation.
comment:8 Changed 3 years ago by
 Commit changed from 74234d4de9bf109ded91c74c0657678317de7bb3 to 7ad95eaececd29f7198a7be267f85312e788c25f
Branch pushed to git repo; I updated commit sha1. New commits:
7ad95ea  Abelian groups with gap are not independent beings of the sage abelian groups

comment:9 Changed 3 years ago by
 Commit changed from 7ad95eaececd29f7198a7be267f85312e788c25f to 99296dc97b86b476760e19f7acabbac2c992c6ed
Branch pushed to git repo; I updated commit sha1. New commits:
99296dc  Reworked pickling. Reference builds now.

comment:10 Changed 3 years ago by
 Commit changed from 99296dc97b86b476760e19f7acabbac2c992c6ed to c3bd7a1652a0022efcf999a84ace4282019cb5f2
Branch pushed to git repo; I updated commit sha1. New commits:
c3bd7a1  Added an example

comment:11 Changed 3 years ago by
 Commit changed from c3bd7a1652a0022efcf999a84ace4282019cb5f2 to 4d2fbe04419a9eed0da4f2a1ccc8ecb5fc7b9256
Branch pushed to git repo; I updated commit sha1. New commits:
4d2fbe0  Added method is_subgroup_of.

comment:12 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:13 Changed 3 years ago by
the __reduce__
methods are necessary. In sage.groups.libgap_wrapper:
"
Your implementation definitely needs to supply
reduce()
: serialize the LibGAP group. Since GAP does not support Python pickles natively, you need to figure out yourself how you can recreate the group from a pickle.
"
comment:14 Changed 3 years ago by
 Commit changed from 4d2fbe04419a9eed0da4f2a1ccc8ecb5fc7b9256 to 1695bd8153a5e00107d3bccdeb5e09c7d2b14660
Branch pushed to git repo; I updated commit sha1. New commits:
1695bd8  bugfixes

comment:15 Changed 3 years ago by
If you look at the diff you can see
\ No newline at end of file
That needs to be fixed
comment:16 Changed 3 years ago by
what about the related #2153 (just resurrected from long ago) ?
comment:17 Changed 3 years ago by
Interesting I was not aware of that. Do you suggest introducing homsets?
comment:18 Changed 3 years ago by
 Commit changed from 1695bd8153a5e00107d3bccdeb5e09c7d2b14660 to c846a0ebcfb29f9bcabc8a333e201e474b983950
Branch pushed to git repo; I updated commit sha1. New commits:
c846a0e  Changed printing of elements and fixed doctests.

comment:19 Changed 3 years ago by
 Commit changed from c846a0ebcfb29f9bcabc8a333e201e474b983950 to ec2c0116c41fc4755f4ecd2376f96e8db1e98d49
Branch pushed to git repo; I updated commit sha1. New commits:
ec2c011  Doctest _gap_init_ is now optional gap_packages

comment:20 Changed 3 years ago by
 Commit changed from ec2c0116c41fc4755f4ecd2376f96e8db1e98d49 to 42182d097cac024a2fad1f784efcce8b0a6f54ef
Branch pushed to git repo; I updated commit sha1. New commits:
42182d0  Merge branch 'develop' of git://trac.sagemath.org/sage into t/24521/abelian_groups_with_gap

comment:21 Changed 3 years ago by
 Branch changed from u/sbrandhorst/abelian_groups_with_gap to public/groups/abelian_groups_with_gap24521
 Commit changed from 42182d097cac024a2fad1f784efcce8b0a6f54ef to 4e2897ad539a42bbc0a60281ef43f3b1bdd82683
 Reviewers set to Travis Scrimshaw
I finally found some time to make it through this ticket. Here is a synopsis of my changes:
 I simplified the overall logic by renaming
AbelianGroupAmbient_gap
toAbelianGroupGap
and made this is the main entry point asUniqueRepresentation
can handle processing input.  I got rid of the
_with_pc
flag that was basically unused except for defining the behavior ofexponents
, but that is much better handled by a subclass IMO.  I only use
Polycyclic
when it is necessary and error out when unable to import. Previously, there was a subtle change of a behavior only really noticeable becausePolycyclic
generators areg1
whereas the usual libgap's aref1
oncegap_packages
was installed.  I fixed a bug where finite subgroups were placed in the category of infinite groups.
 Other misc doc and formatting changes.
New commits:
4e2897a  Fixing bugs and doing some cleanup.

comment:22 Changed 3 years ago by
If my changes are good to you, then positive review.
comment:23 Changed 3 years ago by
 Commit changed from 4e2897ad539a42bbc0a60281ef43f3b1bdd82683 to 23f489a3a09fd2b94edd994f93bd941ecdaa3db6
Branch pushed to git repo; I updated commit sha1. New commits:
23f489a  Improved exponents()

comment:24 followup: ↓ 25 Changed 3 years ago by
Thank you for the careful review. I did not know about __classcall_private__
.
Your changes look good.
Positive review from my side if you are happy with my changes in exponents
.
A question: what is the purpose of
return super(AbelianGroup_gap, self)._coerce_map_from_(S)
in _coerce_map_from_
? I do not quite understand it.
comment:25 in reply to: ↑ 24 Changed 3 years ago by
 Status changed from needs_review to positive_review
Replying to sbrandhorst:
Positive review from my side if you are happy with my changes in
exponents
.
LGTM. Thanks.
A question: what is the purpose of
return super(AbelianGroup_gap, self)._coerce_map_from_(S)
in_coerce_map_from_
? I do not quite understand it.
It makes a call up to the closest base class's implementation of _coerce_map_from_
. In this case, it is most likely calling the default one from Parent
, which simply returns None
. Yet, perhaps at some point in the future, one of the base classes will implement a more generic coercion check that this class can benefit from.
comment:26 Changed 3 years ago by
 Status changed from positive_review to needs_work
sage t long src/sage/groups/abelian_gps/abelian_group_gap.py ********************************************************************** File "src/sage/groups/abelian_gps/abelian_group_gap.py", line 701, in sage.groups.abelian_gps.abelian_group_gap.AbelianGroupSubgroup_gap.__init__ Failed example: G = AbelianGroupGap([2,3,0]) Exception raised: Traceback (most recent call last): File "/mnt/disk/home/release/Sage/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 534, in _run self.compile_and_execute(example, compiler, test.globs) File "/mnt/disk/home/release/Sage/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 937, in compile_and_execute exec(compiled, globs) File "<doctest sage.groups.abelian_gps.abelian_group_gap.AbelianGroupSubgroup_gap.__init__[5]>", line 1, in <module> G = AbelianGroupGap([Integer(2),Integer(3),Integer(0)]) File "sage/misc/classcall_metaclass.pyx", line 330, in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (build/cythonized/sage/misc/classcall_metaclass.c:1590) return cls.classcall(cls, *args, **kwds) File "/mnt/disk/home/release/Sage/local/lib/python2.7/sitepackages/sage/groups/abelian_gps/abelian_group_gap.py", line 599, in __classcall_private__ return super(AbelianGroupGap, cls).__classcall__(cls, generator_orders) File "sage/misc/cachefunc.pyx", line 1059, in sage.misc.cachefunc.CachedFunction.__call__ (build/cythonized/sage/misc/cachefunc.c:6246) w = self.f(*args, **kwds) File "/mnt/disk/home/release/Sage/local/lib/python2.7/sitepackages/sage/structure/unique_representation.py", line 1021, in __classcall__ instance = typecall(cls, *args, **options) File "sage/misc/classcall_metaclass.pyx", line 497, in sage.misc.classcall_metaclass.typecall (build/cythonized/sage/misc/classcall_metaclass.c:2040) return (<PyTypeObject*>type).tp_call(cls, args, kwds) File "/mnt/disk/home/release/Sage/local/lib/python2.7/sitepackages/sage/groups/abelian_gps/abelian_group_gap.py", line 614, in __init__ raise ImportError("unable to import polycyclic package") ImportError: unable to import polycyclic package **********************************************************************
comment:27 Changed 3 years ago by
 Commit changed from 23f489a3a09fd2b94edd994f93bd941ecdaa3db6 to 59b11eda5656ffb0fb61c99f493718521a6fcea6
Branch pushed to git repo; I updated commit sha1. New commits:
59b11ed  added # optional gap_packages to fix doctest

comment:28 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:29 Changed 3 years ago by
Thank you Volker. Let us see what the bot has to say.
comment:30 Changed 3 years ago by
Whoops, my bad. Forgot to add those. Green patchbot => positive review.
comment:31 Changed 3 years ago by
 Status changed from needs_review to positive_review
comment:32 Changed 3 years ago by
 Commit changed from 59b11eda5656ffb0fb61c99f493718521a6fcea6 to 477b66d14b8eceb0ab2d91a6aa36be646dd16526
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
477b66d  Merge branch 'develop' into t/24521/abelian_groups_with_gap

comment:33 Changed 3 years ago by
 Status changed from needs_review to positive_review
I'm guessing there was a merge conflict? Otherwise you did not have to merge in the latest develop
.
comment:34 Changed 3 years ago by
I am just tired to switch between different versions of sage all the time and recompile everything for hours. So I just keep all my branches up to date with the newest beta.
comment:35 Changed 3 years ago by
 Branch changed from public/groups/abelian_groups_with_gap24521 to 477b66d14b8eceb0ab2d91a6aa36be646dd16526
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
First undocumented prototype