Opened 3 years ago
Closed 2 years ago
#24804 closed defect (duplicate)
py3: minor fixes to sage.libs.ntl
Reported by:  embray  Owned by:  

Priority:  major  Milestone:  sageduplicate/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, GitHub, GitLab)  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 3 years ago by
 Status changed from new to needs_review
comment:2 Changed 3 years ago by
comment:3 Changed 3 years ago by
 Status changed from needs_review to needs_work
comment:4 Changed 3 years ago by
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 flexibleenough to take any types that can be ducktyped as listlike.
comment:5 Changed 3 years ago by
 Milestone changed from sage8.2 to sage8.3
comment:6 Changed 3 years ago by
 Milestone changed from sage8.3 to sage8.4
I believe this issue can reasonably be addressed for Sage 8.4.
comment:7 Changed 3 years ago by
 Milestone changed from sage8.4 to sage8.5
comment:8 Changed 3 years ago by
 Branch changed from u/embray/python3/sagelibsntl 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:
42951cb  py3: some care for libs/ntl

comment:9 Changed 3 years ago by
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 3 years ago by
 Status changed from needs_review to needs_work
comment:11 Changed 3 years ago by
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 sequencelike types in a consistent manner.
comment:12 Changed 3 years ago by
 Commit changed from 42951cb51203677b1987737e38f27943cdc3f63f to 31152f9aceb9db428cbaf797e8fbcb00f2ab4e20
Branch pushed to git repo; I updated commit sha1. New commits:
31152f9  trac 24804 fix by using xrange as type

comment:13 Changed 3 years ago by
green bot. I would prefer to keep the general discussion for somewhere else.
comment:14 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:15 followup: ↓ 17 Changed 3 years ago by
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 3 years ago by
yes, but still this is a little progress..
comment:17 in reply to: ↑ 15 ; followup: ↓ 20 Changed 3 years ago by
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 3 years ago by
ISTM the remaining bug is caused by some map
somewhere. I'm looking into it.
comment:19 Changed 3 years ago by
Looking at the Python code and CAPI docs, there is in fact a Clevel PySequence
API: https://docs.python.org/2/capi/sequence.html
I believe it would be very useful to provide some thin wrappers around this to support sequencelike 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 ; followup: ↓ 23 Changed 3 years ago by
comment:21 Changed 3 years ago by
 Reviewers set to Vincent Klein
 Status changed from needs_review to positive_review
comment:22 Changed 3 years ago by
 Status changed from positive_review to needs_work
I am working on an alternative.
comment:23 in reply to: ↑ 20 Changed 3 years ago by
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 3 years ago by
 Commit changed from 31152f9aceb9db428cbaf797e8fbcb00f2ab4e20 to 56b866a7afe1193180ec4fad90f1da983e697697
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

comment:25 followup: ↓ 32 Changed 3 years ago by
 Dependencies set to #26769
 Status changed from needs_work to needs_review
comment:26 Changed 3 years ago by
 Status changed from needs_review to needs_work
failing doctests.. I would prefer to "keep it simplestupid".
comment:27 Changed 3 years ago by
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 illconsidered, 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 3 years ago by
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 nonlocal, however, and the existing code was still somewhat wrong. I'll just fix that up a bit.
comment:29 Changed 3 years ago by
 Commit changed from 56b866a7afe1193180ec4fad90f1da983e697697 to e2e31f35b62a7c8da831205567c8e1cdb43179bd
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
e2e31f3  update some defunct code

comment:30 Changed 3 years ago by
 Milestone changed from sage8.5 to sage8.7
Retargeting some of my tickets (somewhat optimistically for now).
comment:31 Changed 2 years ago by
 Milestone changed from sage8.7 to sage8.8
Moving all my inprogress tickets to 8.8 milestone.
comment:32 in reply to: ↑ 25 Changed 2 years ago by
 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 simpleminded solution. Otherwise, we can give a green light to the underthecarpet solution proposed in #27588
comment:33 Changed 2 years ago by
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 2 years ago by
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 2 years ago by
 Milestone changed from sage8.8 to sage8.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 years ago by
 Milestone changed from sage8.9 to sageduplicate/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 years ago by
 Cc jhpalmieri fbissey added
comment:38 Changed 2 years ago by
 Status changed from needs_review to positive_review
Sounds good to me.
comment:39 Changed 2 years ago by
 Resolution set to duplicate
 Status changed from positive_review to closed
there seems to be some failing doctests
or at least one, see patchbot