Opened 3 years ago
Closed 3 years ago
#28965 closed enhancement (fixed)
enhance GroupLibGAP
Reported by:  Vincent Delecroix  Owned by:  

Priority:  major  Milestone:  sage9.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: 
Description
 fix libgap conversion of elements
 faster group operations
 more methods
Change History (14)
comment:1 Changed 3 years ago by
Branch:  → u/vdelecroix/28965 

Commit:  → 81e7ecf158613727e2fafec293f134a3dfb7a98f 
Status:  new → needs_review 
comment:2 Changed 3 years ago by
Summary:  enhance LibGapGroup → enhance GroupLibGAP 

comment:3 followup: 4 Changed 3 years ago by
Status:  needs_review → 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 Changed 3 years ago by
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
Status:  needs_work → 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 3 years ago by
Reviewers:  → Simon Brandhorst 

comment:8 Changed 3 years ago by
Status:  positive_review → 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 followup: 10 Changed 3 years ago by
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 Changed 3 years ago by
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
sage/groups/abelian_gps/abelian_group.py
line 961962
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
Commit:  81e7ecf158613727e2fafec293f134a3dfb7a98f → 74d7935b3f347cc64fa5e0f2037b14f8a233a111 

Branch pushed to git repo; I updated commit sha1. New commits:
74d7935  doctest compatibility with Python 2

comment:13 Changed 3 years ago by
Status:  needs_work → positive_review 

comment:14 Changed 3 years ago by
Branch:  u/vdelecroix/28965 → 74d7935b3f347cc64fa5e0f2037b14f8a233a111 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
fix libgap conversion of ElementLibGAP
allow raag elements to be constructed from libgap object
rework gens of ParentLibGAP
less indirection in group operations for ElementLibGAP
clean import
more capable libgap mixin and libgap group