Opened 8 weeks ago
Closed 3 weeks ago
#27588 closed enhancement (fixed)
Py3: Fix libs.ntl.ntl_ZZ_pX.pyx doctests.
Reported by:  vklein  Owned by:  

Priority:  major  Milestone:  sage8.8 
Component:  python3  Keywords:  
Cc:  Merged in:  
Authors:  Vincent Klein, Frédéric Chapoton  Reviewers:  Vincent Klein, Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  
Branch:  a93f26b (Commits)  Commit:  a93f26b8372fc2a87dc8ad089d926789c40457ed 
Dependencies:  Stopgaps: 
Description
Fix libs.ntl.ntl_ZZ_pX.pyx doctests for python3. All libs.ntl doctests should pass with this ticket.
Change History (22)
comment:1 Changed 8 weeks ago by
comment:2 Changed 8 weeks ago by
 Branch set to u/vklein/27588
comment:3 Changed 8 weeks ago by
 Commit set to 91bac0e60583ad55f810121e517d2d75c9f72f33
 Status changed from new to needs_review
New commits:
91bac0e  Trac 27588: Py3: Fix libs.ntl.ntl_ZZ_pX.pyx ...

comment:4 Changed 8 weeks ago by
comment:5 Changed 4 weeks ago by
 Branch changed from u/vklein/27588 to public/ticket/24804
 Commit changed from 91bac0e60583ad55f810121e517d2d75c9f72f33 to e2e31f35b62a7c8da831205567c8e1cdb43179bd
maybe we can instead try again my previous proposal in #24804 ?
New commits:
9d1c59f  add a simple issequence implementation

1d7066c  add a special case for the most common case of list or tuple

79d5bdf  py3: some care for libs/ntl

62508e3  py3: replace a troublesome map() in ntl_GF2X._sage_

3407ae3  cover more sequence types here by using issequence

e2e31f3  update some defunct code

comment:6 Changed 4 weeks ago by
 Branch changed from public/ticket/24804 to public/ticket27588
 Commit changed from e2e31f35b62a7c8da831205567c8e1cdb43179bd to bb4cb87a451aef4ce2911127f1ccc159d6166698
here is a simple but working solution
New commits:
bb4cb87  fix for ntl (simple minded but doing the job)

comment:7 Changed 4 weeks ago by
 Commit changed from bb4cb87a451aef4ce2911127f1ccc159d6166698 to cd7b2fc3a291555099f06191d8971c604dd02ae2
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
cd7b2fc  fix for ntl (simple minded but doing the job)

comment:8 Changed 4 weeks ago by
should be good now. I also made a few minor changes in the doc.
comment:9 Changed 4 weeks ago by
comment:11 Changed 4 weeks ago by
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
.
comment:12 Changed 4 weeks ago by
peut etre que ca marcherait mieux en utilisant xrange, je n'ai pas le temps..
comment:13 Changed 4 weeks ago by
Je vais essayer.
comment:14 Changed 4 weeks ago by
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'>
comment:15 Changed 4 weeks ago by
 Commit changed from cd7b2fc3a291555099f06191d8971c604dd02ae2 to 00f92a6a17410a4120f10bb5907a06da5cdfb2d1
Branch pushed to git repo; I updated commit sha1. New commits:
00f92a6  Trac 27588: use xrange instead of range to be ...

comment:16 Changed 4 weeks ago by
 Status changed from needs_work to needs_review
I give positive review to the commits prior to 00f92a6
comment:17 Changed 3 weeks ago by
probably no need for the import of xrange, as we are in a pyx file..
comment:18 Changed 3 weeks ago by
 Commit changed from 00f92a6a17410a4120f10bb5907a06da5cdfb2d1 to a93f26b8372fc2a87dc8ad089d926789c40457ed
Branch pushed to git repo; I updated commit sha1. New commits:
a93f26b  Trac 27588: remove import xrange

comment:19 Changed 3 weeks ago by
Indeed. import xrange
has been removed.
comment:20 Changed 3 weeks ago by
 Reviewers set to Vincent Klein, Frédéric Chapoton
ok, works both in py2 and py3. If you agree, please set to positive
comment:22 Changed 3 weeks ago by
 Branch changed from public/ticket27588 to a93f26b8372fc2a87dc8ad089d926789c40457ed
 Resolution set to fixed
 Status changed from positive_review to closed
Looking at #24804 (which is already in merge confict) commits it doesn't appear to be conflict with this ticket.