Opened 13 months ago

Closed 12 months ago

#28965 closed enhancement (fixed)

enhance GroupLibGAP

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-9.1
Component: group theory Keywords:
Cc: chapoton, tscrim, sbrandhorst Merged in:
Authors: Vincent Delecroix Reviewers: Simon Brandhorst
Report Upstream: N/A Work issues:
Branch: 74d7935 (Commits) Commit: 74d7935b3f347cc64fa5e0f2037b14f8a233a111
Dependencies: Stopgaps:

Description

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

Change History (14)

comment:1 Changed 13 months ago by vdelecroix

  • Branch set to u/vdelecroix/28965
  • Commit set to 81e7ecf158613727e2fafec293f134a3dfb7a98f
  • Status changed from new to needs_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 13 months ago by vdelecroix

  • Summary changed from enhance LibGapGroup to enhance GroupLibGAP

comment:3 follow-up: Changed 13 months ago by sbrandhorst

  • Status changed from needs_review to needs_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 13 months ago by vdelecroix

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 13 months ago by sbrandhorst

  • Status changed from needs_work to positive_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 13 months ago by sbrandhorst

  • Reviewers set to Simon Brandhorst

comment:7 Changed 13 months ago by vdelecroix

Thank you!

comment:8 Changed 13 months ago by vbraun

  • Status changed from positive_review to needs_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 follow-up: Changed 13 months ago by vdelecroix

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 13 months ago by tscrim

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 13 months ago by vdelecroix

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 13 months ago by git

  • Commit changed from 81e7ecf158613727e2fafec293f134a3dfb7a98f to 74d7935b3f347cc64fa5e0f2037b14f8a233a111

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

​74d7935doctest compatibility with Python 2

comment:13 Changed 13 months ago by vdelecroix

  • Status changed from needs_work to positive_review

comment:14 Changed 12 months ago by vbraun

  • Branch changed from u/vdelecroix/28965 to 74d7935b3f347cc64fa5e0f2037b14f8a233a111
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.