py3: minor fixes to sage.libs.ntl

[embray]

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.

[chapoton]

there seems to be some failing doctests

or at least one, see patchbot

[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.

[embray]

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

[chapoton]

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

---

New commits:

​42951cb

py3: some care for libs/ntl

[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?

[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.

[git]

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

​31152f9

trac 24804 fix by using xrange as type

[chapoton]

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

[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
----------------------------------------------------------------------

[chapoton]

yes, but still this is a little progress..

[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...

[embray]

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

[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).

[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.

[embray]

I am working on an alternative.

[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.

[git]

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

[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.

[chapoton]

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

[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.

[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.

[git]

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

​e2e31f3

update some defunct code

[embray]

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

[embray]

Moving all my in-progress tickets to 8.8 milestone.

[chapoton]

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

[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 ?

[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.

[embray]

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).

[chapoton]

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

[jhpalmieri]

Sounds good to me.