Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow libgap integers as indices #23878

Closed
videlec opened this issue Sep 18, 2017 · 12 comments
Closed

allow libgap integers as indices #23878

videlec opened this issue Sep 18, 2017 · 12 comments

Comments

@videlec
Copy link
Contributor

videlec commented Sep 18, 2017

sage: s = 'abcd'
sage: s[libgap(1)]
Traceback (most recent call last):
...
TypeError: string indices must be integers,
not sage.libs.gap.element.GapElement_Integer

Component: interfaces

Author: Vincent Delecroix

Branch/Commit: a102cf1

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/23878

@videlec videlec added this to the sage-8.1 milestone Sep 18, 2017
@videlec
Copy link
Contributor Author

videlec commented Sep 18, 2017

Branch: u/vdelecroix/23878

@videlec
Copy link
Contributor Author

videlec commented Sep 18, 2017

New commits:

5bdbbb023878: allow libgap integers as indices

@videlec
Copy link
Contributor Author

videlec commented Sep 18, 2017

Commit: 5bdbbb0

@jdemeyer
Copy link

comment:2

I would prefer return int(self) instead of duplicating the __int__ code.

@videlec
Copy link
Contributor Author

videlec commented Sep 18, 2017

comment:3

Agreed, but it ends up with a lot of indirections... (though one might hope that Cython actually bypass some of them)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 18, 2017

Changed commit from 5bdbbb0 to a102cf1

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 18, 2017

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

a102cf123878: allow libgap integers as indices

@videlec
Copy link
Contributor Author

videlec commented Sep 18, 2017

comment:5

done

@jdemeyer
Copy link

comment:6

Replying to @videlec:

Agreed, but it ends up with a lot of indirections...

True, but those indirections are not much of an issue since they are in Cython and C.

@jdemeyer
Copy link

comment:7

positive_review if it passes tests.

@jdemeyer
Copy link

Reviewer: Jeroen Demeyer

@vbraun
Copy link
Member

vbraun commented Sep 24, 2017

Changed branch from u/vdelecroix/23878 to a102cf1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants