Opened 3 years ago
Closed 3 years ago
#27588 closed enhancement (fixed)
Py3: Fix libs.ntl.ntl_ZZ_pX.pyx doctests.
Reported by: | vklein | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.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, GitHub, GitLab) | 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 3 years ago by
comment:2 Changed 3 years ago by
- Branch set to u/vklein/27588
comment:3 Changed 3 years 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 3 years ago by
comment:5 Changed 3 years 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 3 years ago by
- Branch changed from public/ticket/24804 to public/ticket-27588
- 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 3 years 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 3 years ago by
should be good now. I also made a few minor changes in the doc.
comment:9 Changed 3 years ago by
comment:11 Changed 3 years 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 3 years ago by
peut etre que ca marcherait mieux en utilisant xrange, je n'ai pas le temps..
comment:13 Changed 3 years ago by
Je vais essayer.
comment:14 Changed 3 years 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 3 years 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 3 years ago by
- Status changed from needs_work to needs_review
I give positive review to the commits prior to 00f92a6
comment:17 Changed 3 years ago by
probably no need for the import of xrange, as we are in a pyx file..
comment:18 Changed 3 years 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 years ago by
Indeed. import xrange
has been removed.
comment:20 Changed 3 years 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 years ago by
- Branch changed from public/ticket-27588 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.