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: 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) 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 vklein

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

comment:2 Changed 8 weeks ago by vklein

  • Branch set to u/vklein/27588

comment:3 Changed 8 weeks ago by vklein

  • Commit set to 91bac0e60583ad55f810121e517d2d75c9f72f33
  • Status changed from new to needs_review

New commits:

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

comment:4 Changed 8 weeks ago by vklein

  • Authors set to Vincent Klein

comment:5 Changed 4 weeks ago by chapoton

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

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

comment:6 Changed 4 weeks ago by chapoton

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

bb4cb87fix for ntl (simple minded but doing the job)

comment:7 Changed 4 weeks ago by git

  • Commit changed from bb4cb87a451aef4ce2911127f1ccc159d6166698 to cd7b2fc3a291555099f06191d8971c604dd02ae2

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)

comment:8 Changed 4 weeks ago by chapoton

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

comment:9 Changed 4 weeks ago by chapoton

  • Authors changed from Vincent Klein to Vincent Klein, Frédéric Chapoton

comment:10 Changed 4 weeks ago by chapoton

  • Status changed from needs_review to needs_work

fails on python2

comment:11 Changed 4 weeks ago by vklein

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 chapoton

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

comment:13 Changed 4 weeks ago by vklein

Je vais essayer.

comment:14 Changed 4 weeks ago by vklein

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 git

  • Commit changed from cd7b2fc3a291555099f06191d8971c604dd02ae2 to 00f92a6a17410a4120f10bb5907a06da5cdfb2d1

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

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

comment:16 Changed 4 weeks ago by vklein

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

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

comment:18 Changed 3 weeks ago by git

  • Commit changed from 00f92a6a17410a4120f10bb5907a06da5cdfb2d1 to a93f26b8372fc2a87dc8ad089d926789c40457ed

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

a93f26bTrac 27588: remove import xrange

comment:19 Changed 3 weeks ago by vklein

Indeed. import xrange has been removed.

comment:20 Changed 3 weeks ago by chapoton

  • Reviewers set to Vincent Klein, Frédéric Chapoton

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

comment:21 Changed 3 weeks ago by vklein

  • Status changed from needs_review to positive_review

I agree.

comment:22 Changed 3 weeks ago by vbraun

  • Branch changed from public/ticket-27588 to a93f26b8372fc2a87dc8ad089d926789c40457ed
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.