Opened 3 years ago
Closed 2 years ago
#24741 closed defect (fixed)
py3: numerous additional fixes for sage.numerical
Reported by:  embray  Owned by:  embray 

Priority:  major  Milestone:  sage8.7 
Component:  python3  Keywords:  
Cc:  Merged in:  
Authors:  Erik Bray  Reviewers:  David Coudert, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  2ebacb6 (Commits, GitHub, GitLab)  Commit:  2ebacb66f40bf6f3b475684c81a3c749cb1daeee 
Dependencies:  Stopgaps: 
Description
Change History (33)
comment:1 Changed 3 years ago by
 Branch changed from +u/embray/python3/sagenumericalbackends/misc to u/embray/python3/sagenumericalbackends/misc
 Commit set to 3cbd09c3640c85970c8066b70ab7f4b7e7442806
 Status changed from new to needs_review
comment:2 Changed 3 years ago by
 Milestone changed from sage8.2 to sage8.3
comment:3 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:5 Changed 2 years ago by
 Milestone changed from sage8.4 to sage8.5
comment:6 Changed 2 years ago by
 Milestone changed from sage8.5 to sage8.7
Retargeting some of my tickets (somewhat optimistically for now).
comment:7 Changed 2 years ago by
red branch
comment:9 Changed 2 years ago by
This branch is such a mess now. I think a lot of the things in it were already fixed. I'm trying to figure it out...
comment:10 Changed 2 years ago by
yes, indeed. That's why I did not manage to merge or rebase it myself..
comment:11 Changed 2 years ago by
 Owner changed from (none) to embray
Okay, I'm working on it. From what I can tell there are still a handful of fixes in here that haven't made it into the mainline yet.
comment:12 Changed 2 years ago by
 Commit changed from 3cbd09c3640c85970c8066b70ab7f4b7e7442806 to 51b38909b8484c7611260dee32b4abe5316ef5af
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
51b3890  Trac #24741: py3: miscellaneous minor fixes, mostly to sage.numerical backends

comment:13 Changed 2 years ago by
 Commit changed from 51b38909b8484c7611260dee32b4abe5316ef5af to bbfc308629fa0b7424ea52df136a928580f3488b
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
bbfc308  Trac #24741: py3: miscellaneous minor fixes, mostly to sage.numerical backends

comment:14 Changed 2 years ago by
 Status changed from needs_work to needs_review
I think this is fine now. With this, the only failures I'm seeing in sage.numerical
are due to __round__
.
comment:15 Changed 2 years ago by
 some changes will likely conflict with #27342 (sorry for the lack of coordination)
 I am worried about the changes in
src/sage/numerical/backends/cvxopt_backend.pyx
,
because we iterate twice over the input, and this will fail if it is an iterator..
comment:16 Changed 2 years ago by
It could check if it's not already a list
and just wrap it in list
if not. That would be superfluous for other sized sequences but there's no better way to deal with that case without #26769, which I still contend is useful.
comment:17 Changed 2 years ago by
Also if that's definitely a problem it's not caught be any tests. Why don't you see if it actually does break, and if so a regression test should be added.
comment:18 Changed 2 years ago by
ok, then. Let us keep this question about list
for somewhere else.
What about the conflict with #27342 ?
comment:19 Changed 2 years ago by
 why using
if type(indices) is not list:
instead ofif not isinstance(indices, list):
?
 in
src/sage/numerical/interactive_simplex_method.py
 names.extend([str(s) for s in self.x()]) + names.extend(str(s) for s in self.x())
otherwise, LGTM
comment:20 Changed 2 years ago by
We want to convert these inputs explicitly to PyListType
typed objects so that Cython can generate efficient code to loop over them. This is why the previous code explicitly typed those arguments as list
. This does effectively the same thing but allows the arguments to be iterables of other types, and then convert them to list
.
comment:21 Changed 2 years ago by
I perfectly understood that you now allow to give an iterable as input, and that it is important to cast to PyListType
.
What I don't understand is the use of type(indices) is not list
instead of isinstance(indices, list)
. Is this a more efficient formulation in Cython or just a stylistic preference ?
comment:22 Changed 2 years ago by
Yes, if you want to check an exact type using type(foo) is bar
is orders of magnitude faster, because it's just a pointer comparison.
isinstance(...)
is a much more complex beast that needs to be able to support things like subclasses as well as __instancecheck_
and __subclasscheck__
. So it's better, especially in Cython code, to do exact type comparisons when that's all you need in a particular case.
comment:23 Changed 2 years ago by
Thanks for the explanation. It's good to know.
The remaining issues with py3 are due to __round__
, and there is this possible minor improvement #comment:19 for names.extend
.
comment:24 Changed 2 years ago by
Hmm, not that it really matters much here either way but I'm not sure comment:19 is necessarily much of an improvement. In most cases I do advocate for generator expressions, but for small collections there's actually more overhead in terms of setting up the generator object and iterating over it than it's worth. For example:
In [1]: x = list(range(4)) In [2]: %%timeit ...: names = [] ...: names.extend(str(_) for _ in x) ...: 100000 loops, best of 3: 2.19 µs per loop In [3]: %%timeit ...: names = [] ...: names.extend([str(_) for _ in x]) ...: 1000000 loops, best of 3: 1.48 µs per loop
You can also see in the list.extend implementation that it has a more optimized code path for the case where it's given a list or tuple and can use the PySequence_Fast
API, as opposed to the more generic path of having to guess what the new size of the list will be (using __length_hint__
if there is one) and slowly iterate over the iterable.
Of course that's all implementation detail and there's no guarantee in the language that this should be faster. By rights the generator expression should be "better" but in practice it isn't, at least for small lists.
comment:25 Changed 2 years ago by
 Reviewers set to David Coudert
comment:26 Changed 2 years ago by
 Status changed from needs_review to needs_work
Thanks for reminding me. Yes, I'll just rebase this on top of #27342.
comment:27 Changed 2 years ago by
red branch, as expected
comment:28 Changed 2 years ago by
All the better then.
comment:29 Changed 2 years ago by
 Commit changed from bbfc308629fa0b7424ea52df136a928580f3488b to 2ebacb66f40bf6f3b475684c81a3c749cb1daeee
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
2ebacb6  Trac #24741: py3: miscellaneous minor fixes, mostly to sage.numerical backends

comment:30 Changed 2 years ago by
 Status changed from needs_work to needs_review
I think this is still good, though I did not test on Python 3.
comment:31 Changed 2 years ago by
 Reviewers changed from David Coudert to David Coudert, Travis Scrimshaw
 Status changed from needs_review to positive_review
With this branch on Python3, I go from
sage t src/sage/numerical/mip.pyx # 1 doctest failed sage t src/sage/numerical/backends/cvxopt_sdp_backend.pyx # 6 doctests failed sage t src/sage/numerical/sdp.pyx # 10 doctests failed sage t src/sage/numerical/backends/cvxopt_backend.pyx # 9 doctests failed sage t src/sage/numerical/backends/glpk_backend.pyx # Killed due to abort
to
sage t src/sage/numerical/backends/cvxopt_backend.pyx # 8 doctests failed sage t src/sage/numerical/backends/cvxopt_sdp_backend.pyx # 5 doctests failed sage t src/sage/numerical/sdp.pyx # 8 doctests failed
which all look like round
type failures
comment:32 Changed 2 years ago by
Same for me. Good to go.
comment:33 Changed 2 years ago by
 Branch changed from u/embray/python3/sagenumericalbackends/misc to 2ebacb66f40bf6f3b475684c81a3c749cb1daeee
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
py3: numerous string encode/decode fixes for sage.numerical.backends
Change the add_col method of backends to accept any iterable instead of strictly list.
Python 3 fixes for the LoggingBackend
Support for tuple unpacking in function signatures was removed in Python 3
py3: miscellaneous minor Python 3 fixes and other cleanup, fixing all the tests for this package on Python 3
For some reason this deprecation warning is shown on Python 2 but not on Python 3