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

Py3: Fix libs.ntl.ntl_ZZ_pX.pyx doctests. #27588

Closed
vinklein mannequin opened this issue Apr 1, 2019 · 31 comments
Closed

Py3: Fix libs.ntl.ntl_ZZ_pX.pyx doctests. #27588

vinklein mannequin opened this issue Apr 1, 2019 · 31 comments

Comments

@vinklein
Copy link
Mannequin

vinklein mannequin commented Apr 1, 2019

Fix libs.ntl.ntl_ZZ_pX.pyx doctests for python3. All libs.ntl doctests should pass with this ticket.

Component: python3

Author: Vincent Klein, Frédéric Chapoton

Branch/Commit: a93f26b

Reviewer: Vincent Klein, Frédéric Chapoton

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

@vinklein vinklein mannequin added this to the sage-8.8 milestone Apr 1, 2019
@vinklein vinklein mannequin added c: python3 labels Apr 1, 2019
@vinklein
Copy link
Mannequin Author

vinklein mannequin commented Apr 1, 2019

comment:1

Looking at #24804 (which is already in merge confict) commits it doesn't appear to be conflict with this ticket.

@vinklein
Copy link
Mannequin Author

vinklein mannequin commented Apr 1, 2019

Branch: u/vklein/27588

@vinklein
Copy link
Mannequin Author

vinklein mannequin commented Apr 1, 2019

Commit: 91bac0e

@vinklein
Copy link
Mannequin Author

vinklein mannequin commented Apr 1, 2019

New commits:

91bac0eTrac 27588: Py3: Fix libs.ntl.ntl_ZZ_pX.pyx ...

@vinklein vinklein mannequin added the s: needs review label Apr 1, 2019
@vinklein
Copy link
Mannequin Author

vinklein mannequin commented Apr 2, 2019

Author: Vincent Klein

@fchapoton
Copy link
Contributor

Changed branch from u/vklein/27588 to public/ticket/24804

@fchapoton
Copy link
Contributor

Changed commit from 91bac0e to e2e31f3

@fchapoton
Copy link
Contributor

comment:5

maybe we can instead try again my previous proposal in #24804 ?


New commits:

9d1c59fadd a simple issequence implementation
1d7066cadd a special case for the most common case of list or tuple
79d5bdfpy3: some care for libs/ntl
62508e3py3: replace a troublesome map() in ntl_GF2X._sage_
3407ae3cover more sequence types here by using issequence
e2e31f3update some defunct code

@fchapoton
Copy link
Contributor

Changed branch from public/ticket/24804 to public/ticket-27588

@fchapoton
Copy link
Contributor

Changed commit from e2e31f3 to bb4cb87

@fchapoton
Copy link
Contributor

comment:6

here is a simple but working solution


New commits:

bb4cb87fix for ntl (simple minded but doing the job)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 1, 2019

Changed commit from bb4cb87 to cd7b2fc

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 1, 2019

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

cd7b2fcfix for ntl (simple minded but doing the job)

@fchapoton
Copy link
Contributor

comment:8

should be good now. I also made a few minor changes in the doc.

@fchapoton
Copy link
Contributor

Changed author from Vincent Klein to Vincent Klein, Frédéric Chapoton

@fchapoton
Copy link
Contributor

comment:10

fails on python2

@vinklein
Copy link
Mannequin Author

vinklein mannequin commented May 2, 2019

comment:11

As range is not a class in python2 the new isinstance is supposed to fail and the cythonize seems to be a little inconsistent if you use isinstance with a tuple as a second argument:
elif isinstance(v, (list, tuple, range)):
generate

__pyx_t_1 = PyObject_IsInstance(__pyx_v_v, __pyx_builtin_range); 
__pyx_t_4 = (__pyx_t_1 != 0);

and
elif isinstance(v, (list, tuple)) or isinstance(v, range): generate

pyx_t_4 = PyObject_IsInstance(__pyx_v_v, __pyx_builtin_range); 
if (unlikely(__pyx_t_4 == ((int)-1))) __PYX_ERR(0, 96, __pyx_L1_error)

The latest is the consistent behaviour for me.

For the errors generated by this ticket with python2 the object return -1 on PyObject_IsInstance(__pyx_v_v, __pyx_builtin_range) and then isinstance(v, (list, tuple, range) return True.

@fchapoton
Copy link
Contributor

comment:12

peut etre que ca marcherait mieux en utilisant xrange, je n'ai pas le temps..

@vinklein
Copy link
Mannequin Author

vinklein mannequin commented May 2, 2019

comment:13

Je vais essayer.

@vinklein
Copy link
Mannequin Author

vinklein mannequin commented May 2, 2019

comment:14

It looks like that work !

with sage python3:

sage: from past.builtins import xrange
sage: isinstance(range(5), xrange)
True
sage: type(xrange(2))
<class 'range'>

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 2, 2019

Changed commit from cd7b2fc to 00f92a6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 2, 2019

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

00f92a6Trac 27588: use xrange instead of range to be ...

@vinklein
Copy link
Mannequin Author

vinklein mannequin commented May 2, 2019

comment:16

I give positive review to the commits prior to 00f92a6

@vinklein vinklein mannequin added s: needs review and removed s: needs work labels May 2, 2019
@fchapoton
Copy link
Contributor

comment:17

probably no need for the import of xrange, as we are in a pyx file..

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 3, 2019

Changed commit from 00f92a6 to a93f26b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 3, 2019

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

a93f26bTrac 27588: remove import xrange

@vinklein
Copy link
Mannequin Author

vinklein mannequin commented May 3, 2019

comment:19

Indeed. import xrange has been removed.

@fchapoton
Copy link
Contributor

comment:20

ok, works both in py2 and py3. If you agree, please set to positive

@fchapoton
Copy link
Contributor

Reviewer: Vincent Klein, Frédéric Chapoton

@vinklein
Copy link
Mannequin Author

vinklein mannequin commented May 3, 2019

comment:21

I agree.

@vbraun
Copy link
Member

vbraun commented May 6, 2019

Changed branch from public/ticket-27588 to a93f26b

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

2 participants