py3: numerous additional fixes for sage.numerical

[embray]

Component: python3

Author: Erik Bray

Branch/Commit: 2ebacb6

Reviewer: David Coudert, Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/24741

[embray]

New commits:

f08fb45	py3: numerous string encode/decode fixes for sage.numerical.backends
9db2b12	Change the add_col method of backends to accept any iterable instead of strictly list.
0b735bf	Python 3 fixes for the LoggingBackend
49846ed	Support for tuple unpacking in function signatures was removed in Python 3
14f355b	py3: miscellaneous minor Python 3 fixes and other cleanup, fixing all the tests for this package on Python 3
3cbd09c	For some reason this deprecation warning is shown on Python 2 but not on Python 3

[embray]

Changed branch from +u/embray/python3/sage-numerical-backends/misc to u/embray/python3/sage-numerical-backends/misc

[embray]

Commit: 3cbd09c

[embray]

comment:3

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

[fchapoton]

comment:4

branch is red

[embray]

comment:6

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

[fchapoton]

comment:7

red branch

[fchapoton]

Changed dependencies from #24740 to none

[fchapoton]

comment:8

still red

[embray]

comment:9

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

[fchapoton]

comment:10

yes, indeed. That's why I did not manage to merge or rebase it myself..

[embray]

comment:11

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.

[sagetrac-git]

Changed commit from 3cbd09c to 51b3890

[sagetrac-git]

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

[embray]

comment:16

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.

[embray]

comment:17

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.

[fchapoton]

comment:18

ok, then. Let us keep this question about list for somewhere else.

What about the conflict with #27342 ?

[dcoudert]

comment:19

• why using if type(indices) is not list: instead of if 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

[embray]

comment:20

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.

[dcoudert]

comment:21

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 ?

[embray]

comment:22

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.

[dcoudert]

comment:23

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.

[embray]

comment:24

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.

[dcoudert]

comment:25

Thank you very much for this detailed explanation. Then I agree that current formulation is good.

To avoid merge conflict with #27342, you should revert some changes

• linear_functions.pyx (same done in py3: some tiny fixes in numerical folder #27342)
• mip.pyx:

-            sage: x_sol.keys()
+            sage: sorted(x_sol)

[dcoudert]

Reviewer: David Coudert

[embray]

comment:26

Thanks for reminding me. Yes, I'll just rebase this on top of #27342.

[fchapoton]

comment:27

red branch, as expected

[embray]

comment:28

All the better then.

[sagetrac-git]

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

[sagetrac-git]

Changed commit from bbfc308 to 2ebacb6

[embray]

comment:30

I think this is still good, though I did not test on Python 3.

[tscrim]

comment:31

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

[tscrim]

Changed reviewer from David Coudert to David Coudert, Travis Scrimshaw

[dcoudert]

comment:32

Same for me. Good to go.

[vbraun]

Changed branch from u/embray/python3/sage-numerical-backends/misc to 2ebacb6