Opened 3 years ago

Closed 3 years ago

#24521 closed enhancement (fixed)

Abelian Groups with Gap

Reported by: sbrandhorst Owned by:
Priority: major Milestone: sage-8.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:

Status badges

Description

Interface our abelian groups with the abelian groups in gap through libgap.

Change History (35)

comment:1 Changed 3 years ago by sbrandhorst

  • Branch set to u/sbrandhorst/abelian_groups_with_gap

comment:2 Changed 3 years ago by tscrim

  • Cc tscrim added
  • Commit set to f9af766f54b7bf3200c7fe55f7da2304bd56a856

New commits:

f9af766First undocumented prototype

comment:3 Changed 3 years ago by git

  • Commit changed from f9af766f54b7bf3200c7fe55f7da2304bd56a856 to 0b13ad7861cddf55f6a5de067ef7541f7d2cac52

Branch pushed to git repo; I updated commit sha1. New commits:

0b13ad7Documentation an bugfixes.

comment:4 Changed 3 years ago by sbrandhorst

  • Status changed from new to needs_review

comment:5 Changed 3 years ago by tscrim

  • Status changed from needs_review to needs_work

Some comments:

  • A number of your 1-line descriptions are missing periods/full-stops at the end.
  • AbelianGroupElement_gap.__reduce__ is missing EXAMPLES:: or TESTS::. (Is this method necessary?)
  • Your INPUT: and OUTPUT: blocks are over-indented.
  • AbelianGroup_gap.__init__ is missing documentation. I would recommend having the TestSuite test there and in the class-level 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 by UniqueRepresentation.
  • _coerce_map_from_ is missing documentation.
  • I do not understand the sage method.
  • The OUTPUT: of subgroup 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 git

  • Commit changed from 0b13ad7861cddf55f6a5de067ef7541f7d2cac52 to 74234d4de9bf109ded91c74c0657678317de7bb3

Branch pushed to git repo; I updated commit sha1. New commits:

74234d4Documentation and removed hash method for AbelianGroup_gap

comment:7 Changed 3 years ago by sbrandhorst

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 git

  • Commit changed from 74234d4de9bf109ded91c74c0657678317de7bb3 to 7ad95eaececd29f7198a7be267f85312e788c25f

Branch pushed to git repo; I updated commit sha1. New commits:

7ad95eaAbelian groups with gap are not independent beings of the sage abelian groups

comment:9 Changed 3 years ago by git

  • Commit changed from 7ad95eaececd29f7198a7be267f85312e788c25f to 99296dc97b86b476760e19f7acabbac2c992c6ed

Branch pushed to git repo; I updated commit sha1. New commits:

99296dcReworked pickling. Reference builds now.

comment:10 Changed 3 years ago by git

  • Commit changed from 99296dc97b86b476760e19f7acabbac2c992c6ed to c3bd7a1652a0022efcf999a84ace4282019cb5f2

Branch pushed to git repo; I updated commit sha1. New commits:

c3bd7a1Added an example

comment:11 Changed 3 years ago by git

  • Commit changed from c3bd7a1652a0022efcf999a84ace4282019cb5f2 to 4d2fbe04419a9eed0da4f2a1ccc8ecb5fc7b9256

Branch pushed to git repo; I updated commit sha1. New commits:

4d2fbe0Added method is_subgroup_of.

comment:12 Changed 3 years ago by sbrandhorst

  • Status changed from needs_work to needs_review

comment:13 Changed 3 years ago by sbrandhorst

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 git

  • Commit changed from 4d2fbe04419a9eed0da4f2a1ccc8ecb5fc7b9256 to 1695bd8153a5e00107d3bccdeb5e09c7d2b14660

Branch pushed to git repo; I updated commit sha1. New commits:

1695bd8bugfixes

comment:15 Changed 3 years ago by vdelecroix

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 chapoton

what about the related #2153 (just resurrected from long ago) ?

comment:17 Changed 3 years ago by sbrandhorst

Interesting I was not aware of that. Do you suggest introducing homsets?

comment:18 Changed 3 years ago by git

  • Commit changed from 1695bd8153a5e00107d3bccdeb5e09c7d2b14660 to c846a0ebcfb29f9bcabc8a333e201e474b983950

Branch pushed to git repo; I updated commit sha1. New commits:

c846a0eChanged printing of elements and fixed doctests.

comment:19 Changed 3 years ago by git

  • Commit changed from c846a0ebcfb29f9bcabc8a333e201e474b983950 to ec2c0116c41fc4755f4ecd2376f96e8db1e98d49

Branch pushed to git repo; I updated commit sha1. New commits:

ec2c011Doctest _gap_init_ is now optional gap_packages

comment:20 Changed 3 years ago by git

  • Commit changed from ec2c0116c41fc4755f4ecd2376f96e8db1e98d49 to 42182d097cac024a2fad1f784efcce8b0a6f54ef

Branch pushed to git repo; I updated commit sha1. New commits:

42182d0Merge branch 'develop' of git://trac.sagemath.org/sage into t/24521/abelian_groups_with_gap

comment:21 Changed 3 years ago by tscrim

  • Branch changed from u/sbrandhorst/abelian_groups_with_gap to public/groups/abelian_groups_with_gap-24521
  • 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 to AbelianGroupGap and made this is the main entry point as UniqueRepresentation can handle processing input.
  • I got rid of the _with_pc flag that was basically unused except for defining the behavior of exponents, 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 because Polycyclic generators are g1 whereas the usual libgap's are f1 once gap_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:

4e2897aFixing bugs and doing some cleanup.

comment:22 Changed 3 years ago by tscrim

If my changes are good to you, then positive review.

comment:23 Changed 3 years ago by git

  • Commit changed from 4e2897ad539a42bbc0a60281ef43f3b1bdd82683 to 23f489a3a09fd2b94edd994f93bd941ecdaa3db6

Branch pushed to git repo; I updated commit sha1. New commits:

23f489aImproved exponents()

comment:24 follow-up: Changed 3 years ago by sbrandhorst

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 tscrim

  • 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 vbraun

  • 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/site-packages/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/site-packages/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/site-packages/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/site-packages/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/site-packages/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 git

  • Commit changed from 23f489a3a09fd2b94edd994f93bd941ecdaa3db6 to 59b11eda5656ffb0fb61c99f493718521a6fcea6

Branch pushed to git repo; I updated commit sha1. New commits:

59b11edadded # optional gap_packages to fix doctest

comment:28 Changed 3 years ago by sbrandhorst

  • Status changed from needs_work to needs_review

comment:29 Changed 3 years ago by sbrandhorst

Thank you Volker. Let us see what the bot has to say.

comment:30 Changed 3 years ago by tscrim

Whoops, my bad. Forgot to add those. Green patchbot => positive review.

comment:31 Changed 3 years ago by tscrim

  • Status changed from needs_review to positive_review

comment:32 Changed 3 years ago by git

  • 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:

477b66dMerge branch 'develop' into t/24521/abelian_groups_with_gap

comment:33 Changed 3 years ago by tscrim

  • 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 sbrandhorst

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 vbraun

  • Branch changed from public/groups/abelian_groups_with_gap-24521 to 477b66d14b8eceb0ab2d91a6aa36be646dd16526
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.