Opened 17 months ago

Closed 4 months 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) Commit: 2ebacb66f40bf6f3b475684c81a3c749cb1daeee
Dependencies: Stopgaps:

Description


Change History (33)

comment:1 Changed 17 months ago by embray

  • 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

New commits:

f08fb45py3: numerous string encode/decode fixes for sage.numerical.backends
9db2b12Change the add_col method of backends to accept any iterable instead of strictly list.
0b735bfPython 3 fixes for the LoggingBackend
49846edSupport for tuple unpacking in function signatures was removed in Python 3
14f355bpy3: miscellaneous minor Python 3 fixes and other cleanup, fixing all the tests for this package on Python 3
3cbd09cFor some reason this deprecation warning is shown on Python 2 but not on Python 3

comment:2 Changed 15 months ago by embray

  • Milestone changed from sage-8.2 to sage-8.3

comment:3 Changed 12 months ago by embray

  • Milestone changed from sage-8.3 to sage-8.4

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

comment:4 Changed 11 months ago by chapoton

  • Status changed from needs_review to needs_work

branch is red

comment:5 Changed 9 months ago by embray

  • Milestone changed from sage-8.4 to sage-8.5

comment:6 Changed 7 months ago by embray

  • Milestone changed from sage-8.5 to sage-8.7

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

comment:7 Changed 7 months ago by chapoton

red branch

comment:8 Changed 5 months ago by chapoton

  • Dependencies #24740 deleted

still red

comment:9 Changed 5 months ago by embray

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 5 months ago by chapoton

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

comment:11 Changed 5 months ago by embray

  • 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 5 months ago by git

  • Commit changed from 3cbd09c3640c85970c8066b70ab7f4b7e7442806 to 51b38909b8484c7611260dee32b4abe5316ef5af

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

51b3890Trac #24741: py3: miscellaneous minor fixes, mostly to sage.numerical backends

comment:13 Changed 5 months ago by git

  • Commit changed from 51b38909b8484c7611260dee32b4abe5316ef5af to bbfc308629fa0b7424ea52df136a928580f3488b

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

bbfc308Trac #24741: py3: miscellaneous minor fixes, mostly to sage.numerical backends

comment:14 Changed 5 months ago by embray

  • 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 5 months ago by chapoton

  • 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 5 months ago by embray

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 5 months ago by embray

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 5 months ago by chapoton

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

What about the conflict with #27342 ?

comment:19 Changed 5 months ago by dcoudert

  • 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

comment:20 Changed 5 months ago by embray

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 5 months ago by dcoudert

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 5 months ago by embray

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 5 months ago by dcoudert

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 5 months ago by embray

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.

Last edited 5 months ago by embray (previous) (diff)

comment:25 Changed 5 months ago by dcoudert

  • Reviewers set to David Coudert

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 #27342)
  • mip.pyx:
    -            sage: x_sol.keys()
    +            sage: sorted(x_sol)
    

comment:26 Changed 5 months ago by embray

  • 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 5 months ago by chapoton

red branch, as expected

comment:28 Changed 4 months ago by embray

All the better then.

comment:29 Changed 4 months ago by git

  • Commit changed from bbfc308629fa0b7424ea52df136a928580f3488b to 2ebacb66f40bf6f3b475684c81a3c749cb1daeee

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

2ebacb6Trac #24741: py3: miscellaneous minor fixes, mostly to sage.numerical backends

comment:30 Changed 4 months ago by embray

  • 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 4 months ago by tscrim

  • 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 4 months ago by dcoudert

Same for me. Good to go.

comment:33 Changed 4 months ago by vbraun

  • Branch changed from u/embray/python3/sage-numerical-backends/misc to 2ebacb66f40bf6f3b475684c81a3c749cb1daeee
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.