Opened 3 years ago

Closed 3 years ago

#28965 closed enhancement (fixed)

enhance GroupLibGAP

Reported by: Vincent Delecroix Owned by:
Priority: major Milestone: sage-9.1
Component: group theory Keywords:
Cc: Frédéric Chapoton, Travis Scrimshaw, Simon Brandhorst Merged in:
Authors: Vincent Delecroix Reviewers: Simon Brandhorst
Report Upstream: N/A Work issues:
Branch: 74d7935 (Commits, GitHub, GitLab) Commit: 74d7935b3f347cc64fa5e0f2037b14f8a233a111
Dependencies: Stopgaps:

Status badges

Description

  • fix libgap conversion of elements
  • faster group operations
  • more methods

Change History (14)

comment:1 Changed 3 years ago by Vincent Delecroix

Branch: u/vdelecroix/28965
Commit: 81e7ecf158613727e2fafec293f134a3dfb7a98f
Status: newneeds_review

New commits:

d90c228fix libgap conversion of ElementLibGAP
d25f239allow raag elements to be constructed from libgap object
03dca2brework gens of ParentLibGAP
2044eb2less indirection in group operations for ElementLibGAP
95c76cbclean import
81e7ecfmore capable libgap mixin and libgap group

comment:2 Changed 3 years ago by Vincent Delecroix

Summary: enhance LibGapGroupenhance GroupLibGAP

comment:3 Changed 3 years ago by Simon Brandhorst

Status: needs_reviewneeds_work

the part on class Element(ArtinGroupElement): looks unrelated. Please move that to a different ticket. Otherwise it looks good to me.

comment:4 in reply to:  3 Changed 3 years ago by Vincent Delecroix

Replying to sbrandhorst:

the part on class Element(ArtinGroupElement): looks unrelated. Please move that to a different ticket. Otherwise it looks good to me.

It is related! Without this, the ArtinGroupElement can not be rebuilt from libgap objects. Without this change, the two tests introduced in commit d90c228 just fail.

comment:5 Changed 3 years ago by Simon Brandhorst

Status: needs_workpositive_review

Whether or not the internal logic is okay, I do not know. The code seems to do what it says. So I am okay.

comment:6 Changed 3 years ago by Simon Brandhorst

Reviewers: Simon Brandhorst

comment:7 Changed 3 years ago by Vincent Delecroix

Thank you!

comment:8 Changed 3 years ago by Volker Braun

Status: positive_reviewneeds_work

On Python 2:

**********************************************************************
File "src/sage/groups/libgap_wrapper.pyx", line 505, in sage.groups.libgap_wrapper.ElementLibGAP.gap
Failed example:
    type(libgap(FreeGroup('a, b').an_element()))
Expected:
    <class 'sage.libs.gap.element.GapElement'>
Got:
    <type 'sage.libs.gap.element.GapElement'>
**********************************************************************
1 item had failures:
   1 of   9 in sage.groups.libgap_wrapper.ElementLibGAP.gap
    [141 tests, 1 failure, 0.59 s]
----------------------------------------------------------------------
sage -t --long src/sage/groups/libgap_wrapper.pyx  # 1 doctest failed
----------------------------------------------------------------------

comment:9 Changed 3 years ago by Vincent Delecroix

Is this supposed to fail? There are tons of doctest with randomly chosen <class X> or <type X>!

$ git grep "<class " | wc -l
1627
$ git grep "<type " | wc -l
937

comment:10 in reply to:  9 Changed 3 years ago by Travis Scrimshaw

Replying to vdelecroix:

Is this supposed to fail? There are tons of doctest with randomly chosen <class X> or <type X>!

It comes from a difference between Python2 and 3, and they are not randomly chosen. However, you can use <... X> to make it compatible with both.

comment:11 Changed 3 years ago by Vincent Delecroix

sage/groups/abelian_gps/abelian_group.py line 961-962

sage: F = AbelianGroup(3, [2], names='abc')
sage: list(map(type, F.gens_orders()))
[<type 'sage.rings.integer.Integer'>,
 <type 'sage.rings.integer.Integer'>,
 <type 'sage.rings.integer.Integer'>]

Which is not the output you get with Python 3. Should I just change it to type then (it makes sage -t happy).

comment:12 Changed 3 years ago by git

Commit: 81e7ecf158613727e2fafec293f134a3dfb7a98f74d7935b3f347cc64fa5e0f2037b14f8a233a111

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

74d7935doctest compatibility with Python 2

comment:13 Changed 3 years ago by Vincent Delecroix

Status: needs_workpositive_review

comment:14 Changed 3 years ago by Volker Braun

Branch: u/vdelecroix/2896574d7935b3f347cc64fa5e0f2037b14f8a233a111
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.