Opened 4 years ago
Closed 3 years 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, 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 4 years ago by
- Status changed from new to needs_review
comment:2 Changed 4 years ago by
comment:3 Changed 4 years ago by
- Status changed from needs_review to needs_work
comment:4 Changed 4 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 flexible-enough to take any types that can be duck-typed as list-like.
comment:5 Changed 4 years ago by
- Milestone changed from sage-8.2 to sage-8.3
comment:6 Changed 4 years ago by
- 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 4 years ago by
- Milestone changed from sage-8.4 to sage-8.5
comment:8 Changed 3 years ago by
- 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:
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 sequence-like 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 follow-up: ↓ 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 ; follow-up: ↓ 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 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: ↓ 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 follow-up: ↓ 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 simple-stupid".
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 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 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 non-local, 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 sage-8.5 to sage-8.7
Retargeting some of my tickets (somewhat optimistically for now).
comment:31 Changed 3 years ago by
- 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 3 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 simple-minded solution. Otherwise, we can give a green light to the under-the-carpet solution proposed in #27588
comment:33 Changed 3 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 3 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 3 years ago by
- 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 3 years ago by
- 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 3 years ago by
- Cc jhpalmieri fbissey added
comment:38 Changed 3 years ago by
- Status changed from needs_review to positive_review
Sounds good to me.
comment:39 Changed 3 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