Opened 4 years ago
Closed 3 years ago
#24741 closed defect (fixed)
py3: numerous additional fixes for sage.numerical
Reported by: | embray | Owned by: | embray |
---|---|---|---|
Priority: | major | Milestone: | sage-8.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 4 years ago by
- Branch changed from +u/embray/python3/sage-numerical-backends/misc to u/embray/python3/sage-numerical-backends/misc
- Commit set to 3cbd09c3640c85970c8066b70ab7f4b7e7442806
- Status changed from new to needs_review
comment:2 Changed 4 years ago by
- Milestone changed from sage-8.2 to sage-8.3
comment:3 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:5 Changed 4 years ago by
- Milestone changed from sage-8.4 to sage-8.5
comment:6 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:7 Changed 3 years ago by
red branch
comment:9 Changed 3 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 3 years ago by
yes, indeed. That's why I did not manage to merge or rebase it myself..
comment:11 Changed 3 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 3 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 3 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 3 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 3 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 3 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 3 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 3 years ago by
ok, then. Let us keep this question about list
for somewhere else.
What about the conflict with #27342 ?
comment:19 Changed 3 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 3 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 3 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 3 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 3 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 3 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 iterator expression should be "better" but in practice it isn't, at least for small lists.
comment:25 Changed 3 years ago by
- Reviewers set to David Coudert
comment:26 Changed 3 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 3 years ago by
red branch, as expected
comment:28 Changed 3 years ago by
All the better then.
comment:29 Changed 3 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 3 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 3 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 3 years ago by
Same for me. Good to go.
comment:33 Changed 3 years ago by
- Branch changed from u/embray/python3/sage-numerical-backends/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