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: sage-7.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:

Status badges

Description (last modified by vdelecroix)

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 and matrix to GapElement_List

Change History (20)

comment:1 follow-up: Changed 6 years ago by vbraun

libgap lists are python-iterable, so __getitem__ needs to start at 0. otherwise you'd have list(x)[0] != x[0]

comment:2 in reply to: ↑ 1 Changed 6 years ago by vdelecroix

Replying to vbraun:

libgap lists are python-iterable, so __getitem__ needs to start at 0. otherwise you'd have list(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 follow-up: Changed 6 years ago by 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?

comment:4 in reply to: ↑ 3 Changed 6 years ago by vdelecroix

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 follow-up: Changed 6 years ago by 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

comment:6 in reply to: ↑ 5 Changed 6 years ago by vdelecroix

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 vdelecroix

  • Authors set to Vincent Delecroix
  • 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:

ee6506dTrac 18158: generic __getitem__ for libgap object
Last edited 6 years ago by vdelecroix (previous) (diff)

comment:8 follow-up: Changed 6 years ago by vbraun

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

  • Commit changed from ee6506d15b2bbb976c738d6b804edf864a6ab24e to c27056e7be55c28870ff781ff431b0fe58d927d5

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

c27056eTrac 18158: more __getitem__/__len__ for libgap

comment:10 in reply to: ↑ 8 Changed 5 years ago by vdelecroix

  • Description modified (diff)
  • Milestone changed from sage-6.6 to sage-7.2

Replying to vbraun:

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

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 dimpase

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 vdelecroix

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 dimpase

Sorry, well, look at your branch yourself. It seems that you have removed and re-inserted 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 vdelecroix

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 git

  • Commit changed from c27056e7be55c28870ff781ff431b0fe58d927d5 to dbf8aeac7bdcb1c458711b9c4d2f57a6d4fbac1c

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

dbf8aeaTrac 18158: simplify some tests

comment:16 Changed 5 years ago by vdelecroix

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

  • Description modified (diff)

comment:18 Changed 5 years ago by vdelecroix

  • Description modified (diff)

Is the description helpful enough? I can revamp the commits if you like.

comment:19 Changed 5 years ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

looks good to me.

comment:20 Changed 5 years ago by vbraun

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