Opened 6 years ago
Closed 5 years ago
#18158 closed defect (fixed)
wrap more libgap objects as `GapElement_List`
Reported by:  vdelecroix  Owned by:  

Priority:  major  Milestone:  sage7.2 
Component:  interfaces  Keywords:  
Cc:  vbraun, dimpase  Merged in:  
Authors:  Vincent Delecroix  Reviewers:  Dima Pasechnik 
Report Upstream:  N/A  Work issues:  
Branch:  dbf8aea (Commits, GitHub, GitLab)  Commit:  dbf8aeac7bdcb1c458711b9c4d2f57a6d4fbac1c 
Dependencies:  Stopgaps: 
Description (last modified by )
Some objects in GAP are not PList but does support indexation.
gap> S := SymmetricGroup(5);; gap> irr := Irr(S);; gap> irr[3][1]; 5
or
gap> gens := GeneratorsOfGroup(SL(2,GF(5)));; gap> m := gens[1];; gap> m[1]; [ Z(5), 0*Z(5) ] gap> m[2]; [ 0*Z(5), Z(5)^3 ]
And Sage currently fails to recognize them as lists
sage: S = libgap.SymmetricGroup(5) sage: irr = S.Irr()[3] sage: irr[0] Traceback (most recent call last): ... TypeError: 'sage.libs.gap.element.GapElement' object has no attribute '__getitem__' sage: gens = libgap.GeneratorsOfGroup(SL(2,GF(5))) sage: m = gens[0] sage: m[0] Traceback (most recent call last): ... TypeError: 'sage.libs.gap.element.GapElement' object has no attribute '__getitem__'
In this branch:
 We change the way gap lists are detected and hence more objects are now wrapped as
GapElement_List
. In particular we make possible the access to elements in the above example.
 we move the methods
vector
andmatrix
toGapElement_List
Change History (20)
comment:1 followup: ↓ 2 Changed 6 years ago by
comment:2 in reply to: ↑ 1 Changed 6 years ago by
Replying to vbraun:
libgap lists are pythoniterable, so
__getitem__
needs to start at 0. otherwise you'd havelist(x)[0] != x[0]
gap lists are Python iterable as well
sage: list(gap.List([1,2,3])) [1, 2, 3]
and having list(x)[0] != x[0]
is not breaking any rule.
comment:3 followup: ↓ 4 Changed 6 years ago by
It does break the rule of least surprise, e.g.
sage: l = some_computation() sage: l [ 1, 2, 3 ] sage: l[0] # guess what?
comment:4 in reply to: ↑ 3 Changed 6 years ago by
Replying to vbraun:
It does break the rule of least surprise, e.g.
sage: l = some_computation() sage: l [ 1, 2, 3 ] sage: l[0] # guess what?
I agree with your example. On the other hand, gap permutations starts at 1 and it is coherent with their lists starting at 1.
More importantly, for who is intended the gap/libgap interface objects (and more generally Sage interface objects):
 everybody
 Sage programmers
 users knowing gap (pari, singular, etc)
 ?
I rarely use ._pari_()
, ._gap_()
when doing actual computations. When I do, it is that Sage is lacking some interface.
Vincent
comment:5 followup: ↓ 6 Changed 6 years ago by
Sage also redefines indexing for PARI objects (PARI starts counting at 1, Sage at 0):
sage: pari("[1,2,3][1]") 1 sage: pari("[1,2,3]")[1] 2
comment:6 in reply to: ↑ 5 Changed 6 years ago by
Replying to jdemeyer:
Sage also redefines indexing for PARI objects (PARI starts counting at 1, Sage at 0):
sage: pari("[1,2,3][1]") 1 sage: pari("[1,2,3]")[1] 2
All right, this is as much consistent as gap vs libgap
sage: pari("[1,2,3]")[1] 2 sage: gp("[1,2,3]")[1] 1
I will just add some warning in the doc and try to implement this __getitem__
support in libgap.
Vincent
comment:7 Changed 6 years ago by
 Branch set to u/vdelecroix/18158
 Commit set to ee6506d15b2bbb976c738d6b804edf864a6ab24e
 Description modified (diff)
 Priority changed from critical to major
 Status changed from new to needs_review
I guess this is far from being optimal but it is at least very useful to me!
Vincent
New commits:
ee6506d  Trac 18158: generic __getitem__ for libgap object

comment:8 followup: ↓ 10 Changed 6 years ago by
 Cc dimpase added
Getitem without length is IMHO useless. The GAP function IsList
just calls IS_LIST
internally, so we should perhaps use that instead of IS_PLIST
to decide when to wrap it in a GapElement_List
comment:9 Changed 5 years ago by
 Commit changed from ee6506d15b2bbb976c738d6b804edf864a6ab24e to c27056e7be55c28870ff781ff431b0fe58d927d5
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
c27056e  Trac 18158: more __getitem__/__len__ for libgap

comment:10 in reply to: ↑ 8 Changed 5 years ago by
 Description modified (diff)
 Milestone changed from sage6.6 to sage7.2
Replying to vbraun:
Getitem without length is IMHO useless. The GAP function
IsList
just callsIS_LIST
internally, so we should perhaps use that instead ofIS_PLIST
to decide when to wrap it in aGapElement_List
Indeed it is much more robust and general. That way libgap vectors and matrices are also considered as lists
sage: M = libgap.eval('SL(2,GF(5))').GeneratorsOfGroup()[1] sage: M [ [ Z(5)^2, Z(5)^0 ], [ Z(5)^2, 0*Z(5) ] ] sage: M[0] [ Z(5)^2, Z(5)^0 ]
comment:11 Changed 5 years ago by
I don't really get the purpose of this ticket; e.g.
m = libgap.eval('[[Z(2^2), Z(2)^0],[0*Z(2), Z(2^2)^2]]')
can just as well be done as follows, without an eval
:
F=libgap.GF(4) a=F.PrimitiveElement() m = libgap([[a,a^0],[0*a,a^2]])
comment:12 Changed 5 years ago by
What are you exactly complaining about? With the branch, I am providing access to __getitem__
and __len__
for more gap objects. What eval
has to do with all of this!?
comment:13 Changed 5 years ago by
Sorry, well, look at your branch yourself. It seems that you have removed and reinserted a big chunk. Namely, that m = libgap.eval('[[Z(2^2), Z(2)^0],[0*Z(2), Z(2^2)^2]]')
is from "green" part, and so I assumed that is a part of the ticket contribution...
comment:14 Changed 5 years ago by
Indeed, I moved matrix
and vector
from GapElement
to GapElement_List
.
Though, I can simplify the line that you don't like.
comment:15 Changed 5 years ago by
 Commit changed from c27056e7be55c28870ff781ff431b0fe58d927d5 to dbf8aeac7bdcb1c458711b9c4d2f57a6d4fbac1c
Branch pushed to git repo; I updated commit sha1. New commits:
dbf8aea  Trac 18158: simplify some tests

comment:16 Changed 5 years ago by
 Description modified (diff)
 Summary changed from `__getitem__` for libGap_Element to wrap more libgap objects as `GapElement_List`
comment:17 Changed 5 years ago by
 Description modified (diff)
comment:18 Changed 5 years ago by
 Description modified (diff)
Is the description helpful enough? I can revamp the commits if you like.
comment:19 Changed 5 years ago by
 Reviewers set to Dima Pasechnik
 Status changed from needs_review to positive_review
looks good to me.
comment:20 Changed 5 years ago by
 Branch changed from u/vdelecroix/18158 to dbf8aeac7bdcb1c458711b9c4d2f57a6d4fbac1c
 Resolution set to fixed
 Status changed from positive_review to closed
libgap lists are pythoniterable, so
__getitem__
needs to start at 0. otherwise you'd havelist(x)[0] != x[0]