py3: operator has no longer div and idiv methods

[fchapoton]

related to #15995

this means that we need to change the coercion framework, and in particular

• src/sage/structure/coerce.pyx
• src/sage/structure/element.pyx

CC: @jdemeyer @tscrim @jhpalmieri @embray

Component: python3

Author: Frédéric Chapoton, Jeroen Demeyer

Branch/Commit: 9f1c2e6

Reviewer: Frédéric Chapoton, Jeroen Demeyer

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

[fchapoton]

Commit: 9d29550

[fchapoton]

Branch: u/chapoton/22875

[fchapoton]

New commits:

9d29550

py3: changing from div to truediv

[fchapoton]

Author: Frédéric Chapoton

[jdemeyer]

Replying to @fchapoton:

this means that we need to change the coercion framework

You don't have to. You could keep using operator.div in Python 2. I would prefer the code to be something like

try:
    from operator import div, idiv
except ImportError:
    pass

Please revert the change to lazy import, it conflicts with #22755.

[sagetrac-git]

Changed commit from 9d29550 to dbc88e5

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

dbc88e5

trac 22875 unchange lazy_import, change one block to TESTS::

[fchapoton]

Dependencies: #22755

[sagetrac-git]

Changed commit from dbc88e5 to e637ced

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

e637ced

remove imports and traces of div and idiv

[fchapoton]

comment:7

maybe this does not really depend on #22755 ?

[jdemeyer]

Changed dependencies from #22755 to none

[jdemeyer]

comment:8

Replying to @fchapoton:

maybe this does not really depend on #22755 ?

No, it doesn't.

[fchapoton]

comment:9

thanks. So let me put this into "needs_review" and hope that a patchbot will run on it.

[fchapoton]

comment:10

well, patchbot is morally green. Please review

[jhpalmieri]

comment:11

I'm confused by the Python 2-to-3 conversion here. When I look at documentation for the Python 2 operator module, I see for operator.truediv:

Return a / b when __future__.division is in effect. This is also known as “true” division.

Does this mean that we need from future import division everywhere, or is the single import in sage.all enough?

Regarding the change

         cdef long j
         cdef long prec
 
-        if op not in (operator.add, operator.sub, operator.mul, operator.div):
+        if op not in (operator.add, operator.sub, operator.mul, operator.truediv):
             raise ValueError("op must be operator.OP where OP=add, sub, mul or div")
 
         if not isinstance(p2, Polynomial):

in sage/rings/polynomial/polynomial_element.pyx, the docstring needs to be changed, too:

        - ``op`` -- ``operator.OP`` where ``OP=add`` or ``sub`` or ``mul`` or
          ``div``.

Also the string in the error message: "op must be ... or truediv".

[sagetrac-git]

Changed commit from e637ced to bf738fe

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

b2805ff	Merge branch 'u/chapoton/22875' in 8.0.b10
bf738fe	trac 2275 truediv in doc

[fchapoton]

comment:13

• in python3, div does no longer exist

• in python2, the behaviour of truediv is the same as in python3

• the future import transforms the behaviour of div and / in python2 to become the same as truediv

The strategy here is very simple : get rid of any use of div everywhere, and use truediv instead. This works smoothly, as the bot has been green already.

This does not fix completely the division issue, as we may have later to add the future import in some files that use /. This has already been partially done.

[fchapoton]

comment:14

green bot !

[jdemeyer]

comment:15

Replying to @jhpalmieri:

I'm confused by the Python 2-to-3 conversion here. When I look at documentation for the Python 2 operator module, I see for operator.truediv:

Return a / b when __future__.division is in effect. This is also known as “true” division.

You need to interpret it "Return a/b as if __future__.division is in effect" (I agree that the documentation is a bit confusing). It does true division regardless of any future imports. Analogously, operator.div does old-style division regardless of any futute imports.

[jdemeyer]

comment:16

1. src/sage/ext/fast_callable.pyx: I guess you should support both div and truediv (instead of only truediv).

2. src/sage/interfaces/maxima_lib.py: same as 1

3. src/sage/rings/integer.pyx: it is wrong to call operator.truediv from __div__. Either call div from __div__ or truediv from __truediv__.

4. src/sage/rings/polynomial/polynomial_element.pyx: composed_op: same as 1

5. src/sage/rings/qqbar.py: add from future import division such that the semantics remain compatible.

6. src/sage/rings/rational.pyx: same as 3

7. src/sage/rings/real_lazy.pyx: same as 5

8. src/sage/structure/coerce.pyx: keep the doctests involving operator.div on Python 2. We still want that to work on Python 2.

9. src/sage/structure/coerce_actions.pyx: same as 8

10. src/sage/structure/element.pyx: this is really wrong: from operator import truediv as div. "truediv" and "div" are distinct operators which should be handled separately.

I don't really understand the symbolic stuff, so I cannot really check it well. On the other hard, there is probably no reason for symbolics to treat "div" differently from "truediv".

[sagetrac-git]

Changed commit from bf738fe to a3833a6

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

275e7a3

trac 22875 adding future division import in 2 files

[sagetrac-git]

Changed commit from a3833a6 to 275e7a3

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

bd248d3	Merge branch 'u/chapoton/22875' in 8.1.b3
ccbbd22	change in fast_callable.pyx

[sagetrac-git]

Changed commit from 275e7a3 to ccbbd22

[embray]

comment:20

Working on getting a working import sage.all on Python 3 and this is one of the fixes needed (among others). I can work on the needed updates if you want.

[fchapoton]

comment:21

Cool ! This is what I was trying to do, see the branch

public/python3-experiment-v1

for nearly the latest stage that I reached.

[jdemeyer]

comment:22

I'll push a reviewer commit, which is easier than telling you what to fix.

[embray]

comment:23

Replying to @fchapoton:

Cool ! This is what I was trying to do, see the branch

public/python3-experiment-v1

for nearly the latest stage that I reached.

Ah, I wasn't sure what your latest branch was. I was confused because there weren't tickets for some things yet, but I was pretty sure you had something closer to working somewhere...

[fchapoton]

comment:24

I went slightly further, in particular by adding __hash__ in one Complex Field class somewhere. I do have a branch, I think.

[jdemeyer]

Changed branch from u/chapoton/22875 to u/jdemeyer/22875

[jdemeyer]

Changed author from Frédéric Chapoton to Frédéric Chapoton, Jeroen Demeyer

[jdemeyer]

comment:26

These are my proposed fixes. Essentially, all fixes are to keep support for Python 2 division where applicable. I reintroduced some instances of operator.div, but all those are guarded in a try/except or are in __div__ which exists only in Python 2.

For me, this ticket can get positive review if you agree with my commit.

---

New commits:

6f86324

Keep support for old-style division where applicable

[jdemeyer]

Changed commit from ccbbd22 to 6f86324

[fchapoton]

comment:27

one failing doctest ('div' gets duplicated in this list, somehow)

[sagetrac-git]

Changed commit from 6f86324 to 9f1c2e6

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

9f1c2e6

builtin_functions now contains two 'div' functions

[jdemeyer]

comment:29

Fixed.

[fchapoton]

comment:30

Looks good to me. I am ready to set to positive. Thanks a lot for finishing the job !

But maybe one should have more patchbot reports, including some with more optional packages installed, just to be sure that nothing serious breaks ?

[fchapoton]

Reviewer: Frédéric Chapoton, Jeroen Demeyer

[fchapoton]

comment:31

ok, there are two green bots. I am setting to positive.

[vbraun]

Changed branch from u/jdemeyer/22875 to 9f1c2e6