Opened 19 months ago

Closed 2 months ago

#24804 closed defect (duplicate)

py3: minor fixes to sage.libs.ntl

Reported by: embray Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: python3 Keywords: ntl
Cc: vklein, jhpalmieri, fbissey Merged in:
Authors: Erik Bray Reviewers: Vincent Klein
Report Upstream: N/A Work issues:
Branch: public/ticket/24804 (Commits) Commit: e2e31f35b62a7c8da831205567c8e1cdb43179bd
Dependencies: #26769 Stopgaps:

Description

Two unrelated but (I think) small updates in sage.libs.ntl. With these fixes (among others in unrelated modules) all the tests pass for sage.libs.ntl in my Python 3 branch.

The update to ntl_ZZ_pX allows it to take as an argument any object that implements the Sequence protocol, instead of just (list, tuple) exactly. To summarize, this means classes that implement at least __iter__, __len__, and __getitem__ (though to be clear, dict is is not a Sequence). This means, among other things, it can accept range objects without any additional fuss, and there were several examples of this usage in the tests.

Change History (39)

comment:1 Changed 19 months ago by embray

  • Status changed from new to needs_review

comment:2 Changed 19 months ago by chapoton

there seems to be some failing doctests

Version 0, edited 19 months ago by chapoton (next)

comment:3 Changed 19 months ago by chapoton

  • Status changed from needs_review to needs_work

comment:4 Changed 19 months ago by embray

Yeah, the SequenceABC change might need a closer examination; see also #24815. I still believe it's basically the right way to go, but Jeroen has pointed out some possible performance impact here. I'm open to other ideas as long as the're flexible-enough to take any types that can be duck-typed as list-like.

comment:5 Changed 17 months ago by embray

  • Milestone changed from sage-8.2 to sage-8.3

comment:6 Changed 14 months ago by embray

  • Milestone changed from sage-8.3 to sage-8.4

I believe this issue can reasonably be addressed for Sage 8.4.

comment:7 Changed 11 months ago by embray

  • Milestone changed from sage-8.4 to sage-8.5

comment:8 Changed 10 months ago by chapoton

  • Branch changed from u/embray/python3/sage-libs-ntl to public/ticket/24804
  • Commit changed from 710733024789049a9611bee15e1c37cfe27a1f86 to 42951cb51203677b1987737e38f27943cdc3f63f
  • Status changed from needs_work to needs_review

in order to move on, I propose a less controversial fix


New commits:

42951cbpy3: some care for libs/ntl

comment:9 Changed 10 months ago by embray

This is still pretty arbitrary. I think it would be nice to at least have defined in one place a tuple of all the common sequence types both built into Python and maybe from Sage as well (e.g. vectors).

In lots of other places we have not explicitly added support for range arguments, so why in this one place?

comment:10 Changed 10 months ago by chapoton

  • Status changed from needs_review to needs_work

comment:11 Changed 10 months ago by embray

I mean, if this specific fix will help make progress I'm not opposed to it, but then we should open a ticket to discuss some kind of general solution to handling sequence-like types in a consistent manner.

comment:12 Changed 10 months ago by git

  • Commit changed from 42951cb51203677b1987737e38f27943cdc3f63f to 31152f9aceb9db428cbaf797e8fbcb00f2ab4e20

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

31152f9trac 24804 fix by using xrange as type

comment:13 Changed 10 months ago by chapoton

green bot. I would prefer to keep the general discussion for somewhere else.

comment:14 Changed 10 months ago by chapoton

  • Status changed from needs_work to needs_review

comment:15 follow-up: Changed 10 months ago by vklein

With these fixes (among others in unrelated modules) all the tests pass for sage.libs.ntl

It's currently false. There is still errors in sage.libs.ntl with this ticket:

----------------------------------------------------------------------
sage -t --long src/sage/libs/ntl/ntl_GF2X.pyx  # 2 doctests failed
sage -t --long src/sage/libs/ntl/ntl_mat_GF2E.pyx  # 1 doctest failed
sage -t --long src/sage/libs/ntl/ntl_GF2E.pyx  # 1 doctest failed
----------------------------------------------------------------------

comment:16 Changed 10 months ago by chapoton

yes, but still this is a little progress..

comment:17 in reply to: ↑ 15 ; follow-up: Changed 10 months ago by embray

Replying to vklein:

With these fixes (among others in unrelated modules) all the tests pass for sage.libs.ntl

It's currently false. There is still errors in sage.libs.ntl with this ticket:

----------------------------------------------------------------------
sage -t --long src/sage/libs/ntl/ntl_GF2X.pyx  # 2 doctests failed
sage -t --long src/sage/libs/ntl/ntl_mat_GF2E.pyx  # 1 doctest failed
sage -t --long src/sage/libs/ntl/ntl_GF2E.pyx  # 1 doctest failed
----------------------------------------------------------------------

"With these fixes (among others in unrelated modules)" the parenthesized part is important: these were fixes that I needed in sage.libs.ntl itself for those tests to pass, but this required some other fixes elsewhere as well. I don't remember what those fixes would have been; this ticket was opened nine months ago. I'll take another look though. I haven't updated my Python 3 branch in too long...

comment:18 Changed 10 months ago by embray

ISTM the remaining bug is caused by some map somewhere. I'm looking into it.

comment:19 Changed 10 months ago by embray

Looking at the Python code and C-API docs, there is in fact a C-level PySequence API: https://docs.python.org/2/c-api/sequence.html

I believe it would be very useful to provide some thin wrappers around this to support sequence-like types generically, but more efficiently than going through the full isinstance check and ABC support (which is something I think still worth looking into making faster).

comment:20 in reply to: ↑ 17 ; follow-up: Changed 10 months ago by vklein

Replying to embray:

Replying to vklein:

With these fixes (among others in unrelated modules) all the tests pass for

the parenthesized part is important: these were fixes that I needed in sage.libs.ntl itself for those tests to pass, but this required some other fixes elsewhere as well.

Ok my bad.

comment:21 Changed 10 months ago by vklein

  • Reviewers set to Vincent Klein
  • Status changed from needs_review to positive_review

comment:22 Changed 10 months ago by embray

  • Status changed from positive_review to needs_work

I am working on an alternative.

comment:23 in reply to: ↑ 20 Changed 10 months ago by embray

Replying to vklein:

Replying to embray:

Replying to vklein:

With these fixes (among others in unrelated modules) all the tests pass for

the parenthesized part is important: these were fixes that I needed in sage.libs.ntl itself for those tests to pass, but this required some other fixes elsewhere as well.

Ok my bad.

Apparently not really: The issue here is a map() call in sage.libs.ntl which could easily be converted to a list comprehension. I think in my python3 branch it still worked because I changed some other interfaces to accept arbitrary iterables better, which is also just as good, but I think these map() calls could just as well be rewritten too.

comment:24 Changed 10 months ago by git

  • Commit changed from 31152f9aceb9db428cbaf797e8fbcb00f2ab4e20 to 56b866a7afe1193180ec4fad90f1da983e697697

Branch pushed to git repo; I updated commit sha1. This was a forced push. 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
56b866aremove some defunct code

comment:25 follow-up: Changed 10 months ago by embray

  • Dependencies set to #26769
  • Status changed from needs_work to needs_review

Updated to take advantage of #26769. If it takes too long to agree on #26769 or something like it then let's revert back. But I think this should work well.

comment:26 Changed 10 months ago by chapoton

  • Status changed from needs_review to needs_work

failing doctests.. I would prefer to "keep it simple-stupid".

comment:27 Changed 10 months ago by embray

It looks like the last failures were caused by the raise TypeError I added in https://git.sagemath.org/sage.git/commit/?id=56b866a7afe1193180ec4fad90f1da983e697697

I just didn't understand that in that code it seems to assume that it's okay to initialize a ntl_ZZ_pX without actually initializing its .x attribute. It struck me as ill-considered, but it at least seems that it was by design (at first glance it just looked like an oversight) so I'll revert that part for now.

comment:28 Changed 10 months ago by embray

It looks like I was partially wrong too about there being no tests for initializing that type from a string. The one test there was for it was very non-local, however, and the existing code was still somewhat wrong. I'll just fix that up a bit.

comment:29 Changed 10 months ago by git

  • Commit changed from 56b866a7afe1193180ec4fad90f1da983e697697 to e2e31f35b62a7c8da831205567c8e1cdb43179bd

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

e2e31f3update some defunct code

comment:30 Changed 9 months ago by embray

  • Milestone changed from sage-8.5 to sage-8.7

Retargeting some of my tickets (somewhat optimistically for now).

comment:31 Changed 6 months ago by embray

  • Milestone changed from sage-8.7 to sage-8.8

Moving all my in-progress tickets to 8.8 milestone.

comment:32 in reply to: ↑ 25 Changed 5 months ago by chapoton

  • Cc vklein added

Replying to embray:

Updated to take advantage of #26769. If it takes too long to agree on #26769 or something like it then let's revert back. But I think this should work well.

Now seems to be a good time to revert back to the simple-minded solution. Otherwise, we can give a green light to the under-the-carpet solution proposed in #27588

comment:33 Changed 5 months ago by chapoton

Erik, after 5 months and no review for your proposal, would you oppose to revert to my previous branch public/ticket/24804, and keep your enhanced solution for later ?

comment:34 Changed 5 months ago by embray

Could you open a new ticket instead? It's fine be me. Of course someone could just review #26769 and come to a definitive conclusion on that and/or offer an alternative solution to the general problem.

comment:35 Changed 3 months ago by embray

  • Milestone changed from sage-8.8 to sage-8.9

Tickets still needing working or clarification should be moved to the next release milestone at the soonest (please feel free to revert if you think the ticket is close to being resolved).

comment:36 Changed 2 months ago by chapoton

  • Milestone changed from sage-8.9 to sage-duplicate/invalid/wontfix
  • Status changed from needs_work to needs_review

I propose to close this one, as all tests pass in libs/ntl

comment:37 Changed 2 months ago by chapoton

  • Cc jhpalmieri fbissey added

comment:38 Changed 2 months ago by jhpalmieri

  • Status changed from needs_review to positive_review

Sounds good to me.

comment:39 Changed 2 months ago by chapoton

  • Resolution set to duplicate
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.