Opened 15 months ago

Last modified 3 weeks ago

#24804 needs_work defect

py3: minor fixes to sage.libs.ntl

Reported by: embray Owned by:
Priority: major Milestone: sage-8.8
Component: python3 Keywords: ntl
Cc: vklein 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 (34)

comment:1 Changed 15 months ago by embray

  • Status changed from new to needs_review

comment:2 Changed 15 months ago by chapoton

there seems to be some failing doctests

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

comment:3 Changed 15 months ago by chapoton

  • Status changed from needs_review to needs_work

comment:4 Changed 15 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 13 months ago by embray

  • Milestone changed from sage-8.2 to sage-8.3

comment:6 Changed 10 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 7 months ago by embray

  • Milestone changed from sage-8.4 to sage-8.5

comment:8 Changed 6 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 6 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 6 months ago by chapoton

  • Status changed from needs_review to needs_work

comment:11 Changed 6 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 6 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 6 months ago by chapoton

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

comment:14 Changed 6 months ago by chapoton

  • Status changed from needs_work to needs_review

comment:15 follow-up: Changed 6 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 6 months ago by chapoton

yes, but still this is a little progress..

comment:17 in reply to: ↑ 15 ; follow-up: Changed 6 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 6 months ago by embray

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

comment:19 Changed 6 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 6 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 6 months ago by vklein

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

comment:22 Changed 6 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 6 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 6 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 6 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 6 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 6 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 6 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 6 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 5 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 8 weeks 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 6 weeks 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 3 weeks 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 3 weeks 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.

Note: See TracTickets for help on using tickets.