Opened 5 years ago

Closed 5 years ago

#18578 closed enhancement (fixed)

Python 3 preparation: Special function __div__() is used no more in Py3

Reported by: wluebbe Owned by: wluebbe
Priority: major Milestone: sage-7.0
Component: misc Keywords: python3
Cc: Merged in:
Authors: Wilfried Luebbe, Jeroen Demeyer Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: 1d0d015 (Commits) Commit: 1d0d01573f63702e802b59633d17ae8dc78bbb98
Dependencies: Stopgaps:

Description

There (among others) the three special functions __div__(), __truediv__() and __floordiv__().

They are invoked by the division operators / and //:

  • In Py2: __div__() and __floordiv__() are called.
  • In Py2 (when from __future__ import division is in effect): __truediv__() and __floordiv__() are called.
  • In Py3: __truediv__() and __floordiv__() are called.

So in the last two cases __div__() is not called for / but __truediv__(). If __truediv__() is not available an exception is thrown.

Therefor classes defining __div__() must be checked that they also work in the last two cases! The related special function __idiv__() is effected too.

The ticket is tracked as a dependency of #15995.

Attachments (2)

test__div__.py (2.1 KB) - added by wluebbe 5 years ago.
Script illustrating div() et.al. with Py2 (and from future import division) and Py3
grep-of-div__.txt (11.2 KB) - added by wluebbe 5 years ago.
The result of git grep -P "div" >~/grep-of-div.txt

Download all attachments as: .zip

Change History (39)

Changed 5 years ago by wluebbe

Script illustrating div() et.al. with Py2 (and from future import division) and Py3

comment:1 Changed 5 years ago by wluebbe

To observe the actual code behavior (besides reading the Python docs) I created a small test script (see the attached file). A summary of the output under various conditions is here:

__div__       is defined
__truediv__   is defined
__floordiv__  is defined
__idiv__      is defined
__itruediv__  is defined
__ifloordiv__ is defined

python version: 2.7.10 (default, May 23 2015, 09:44:00) [MSC v.1500 64 bit (AMD64)]
from __future__ import division is NOT active
function: __div__       115 10 ## operator is /; result=cla(11)
function: __floordiv__  115 10 ## operator is //; result=cla(11)
function: __idiv__      215 10 ## operator is /=; result=cla(21)
function: __ifloordiv__ 215 10 ## operator is //=; result=cla(21)

Python version: 2.7.10 (default, May 23 2015, 09:44:00) [MSC v.1500 64 bit (AMD64)]
from __future__ import division is active
function: __truediv__   115 10 ## operator is /; result=cla(11.5)
function: __floordiv__  115 10 ## operator is //; result=cla(11)
function: __itruediv__  215 10 ## operator is /=; result=cla(21.5)
function: __ifloordiv__ 215 10 ## operator is //=; result=cla(21)

Python version: 3.4.3 (v3.4.3:9b73f1c3e601, Feb 24 2015, 22:44:40) [MSC v.1600 64 bit (AMD64)]
from __future__ import division is NOT active
function: __truediv__   115 10 ## operator is /; result=cla(11.5)
function: __floordiv__  115 10 ## operator is //; result=cla(11)
function: __itruediv__  215 10 ## operator is /=; result=cla(21.5)
function: __ifloordiv__ 215 10 ## operator is //=; result=cla(21)

Python version: 3.4.3 (v3.4.3:9b73f1c3e601, Feb 24 2015, 22:44:40) [MSC v.1600 64 bit (AMD64)]
from __future__ import division is active
function: __truediv__   115 10 ## operator is /; result=cla(11.5)
function: __floordiv__  115 10 ## operator is //; result=cla(11)
function: __itruediv__  215 10 ## operator is /=; result=cla(21.5)
function: __ifloordiv__ 215 10 ## operator is //=; result=cla(21)

Changed 5 years ago by wluebbe

The result of git grep -P "div" >~/grep-of-div.txt

comment:2 Changed 5 years ago by wluebbe

  • Branch set to u/wluebbe/18578
  • Commit set to 9701674d59213529ef6476ecc973a8f2cf69bfb3
  • Owner changed from (none) to wluebbe

Currently the branch contains the changes of 20 .py modules. I start working on the .pyx modules now.


New commits:

9701674Trac #18578: Add special function __truediv__() for Py3

comment:3 follow-up: Changed 5 years ago by wluebbe

But trying the same approach does not seem to work for (e.g.) src/sage/ext/fast_callable.pyx.

    def __truediv__(s, o):
    ...
    ...
    # for Python 2 without from __future__ import division
    __div__ = __truediv__

apparently does not define __div__().

comment:4 in reply to: ↑ 3 Changed 5 years ago by jdemeyer

Replying to wluebbe:

But trying the same approach does not seem to work for (e.g.) src/sage/ext/fast_callable.pyx.

    def __truediv__(s, o):
    ...
    ...
    # for Python 2 without from __future__ import division
    __div__ = __truediv__

apparently does not define __div__().

A cdef class in Cython is really a different thing than a Python class. So it's normal that the same approach doesn't work.

You could (and should) use the __div__ = __truediv__ trick for a class defined in Cython code.

comment:5 Changed 5 years ago by jdemeyer

How about this in Cython?

from cpython.number cimport PyNumber_TrueDivide

def __div__(self, other):
    return PyNumber_TrueDivide(self, other)

comment:6 follow-up: Changed 5 years ago by wluebbe

That gives a compile error:

Cythonizing sage/ext/fast_callable.pyx

Error compiling Cython file:
------------------------------------------------------------
...
            div(1, v_0)
        """
        return _expression_binop_helper(s, o, op_div)

    # for Python 2 without from __future__ import division
    from cpython.number cimport PyNumber_TrueDivide
   ^
------------------------------------------------------------

sage/ext/fast_callable.pyx:917:4: cimport only allowed at module level

Error compiling Cython file:
------------------------------------------------------------
...
        return _expression_binop_helper(s, o, op_div)

    # for Python 2 without from __future__ import division
    from cpython.number cimport PyNumber_TrueDivide
    def __div__(self, other):
        return PyNumber_TrueDivide(self, other)
                                 ^
------------------------------------------------------------

sage/ext/fast_callable.pyx:919:34: undeclared name not builtin: PyNumber_TrueDivide

Copying the code from def __div__ to def ___truediv__ clearly works - but there must be a better way!

comment:7 in reply to: ↑ 6 Changed 5 years ago by jdemeyer

Replying to wluebbe:

That gives a compile error:

sage/ext/fast_callable.pyx:917:4: cimport only allowed at module level

Well, that's because you need to write

... cimport ...

cdef class ...

instead of

cdef class ...

    ... cimport ...

comment:8 Changed 5 years ago by wluebbe

Yes, that did the trick.

I was thinking that I could do without learning Cython ... but ... sigh ...

Thanks for the help!

comment:9 follow-up: Changed 5 years ago by wluebbe

Not a complete success :-( . I am going into a never ending recursion:

    RuntimeError: maximum recursion depth exceeded while calling a Python object
**********************************************************************
1 item had failures:
   2 of   9 in sage.ext.fast_callable.Expression.__truediv__

I am just looking for a way that __div__ directly delegates the work to __truediv__? Calling PyNumber_TrueDivide seems to cause the recursion. How can I call the directly previously defined method __truediv__(s, o) from within def __div__(s,o):?

comment:10 Changed 5 years ago by jdemeyer

I would be easier if you just show me what you have done.

comment:11 Changed 5 years ago by wluebbe

My current changes are in u/wluebbe/18578-1. Meanwhile I will have a look at the Cython doc.

comment:12 in reply to: ↑ 9 Changed 5 years ago by jdemeyer

Replying to wluebbe:

Not a complete success :-( . I am going into a never ending recursion:

    RuntimeError: maximum recursion depth exceeded while calling a Python object
**********************************************************************
1 item had failures:
   2 of   9 in sage.ext.fast_callable.Expression.__truediv__

If you look at the traceback, you see why:

      File "sage/ext/fast_callable.pyx", line 918, in sage.ext.fast_callable.Expression.__div__ (build/cythonized/sage/ext/fast_callable.c:7203)
        return PyNumber_TrueDivide(self, other)
      File "sage/structure/element.pyx", line 2025, in sage.structure.element.RingElement.__truediv__ (build/cythonized/sage/structure/element.c:18064)
        return self.__div__(right)
      File "sage/structure/element.pyx", line 2038, in sage.structure.element.RingElement.__div__ (build/cythonized/sage/structure/element.c:18205)
        return coercion_model.bin_op(self, right, div)
      File "sage/structure/coerce.pyx", line 1040, in sage.structure.coerce.CoercionModel_cache_maps.bin_op (build/cythonized/sage/structure/coerce.c:9052)
        res = mul_method(x)

The problem is caused by this code in element.pyx:

    def __truediv__(self, right):
        # in sage all divs are true
        if not isinstance(self, Element):
            return coercion_model.bin_op(self, right, div)
        return self.__div__(right)

comment:13 Changed 5 years ago by wluebbe

I see -- and element.pyx is also on my todo list. But that code is not the easiest to understand :-/

comment:14 Changed 5 years ago by jdemeyer

  • Dependencies set to #18622

comment:15 Changed 5 years ago by jdemeyer

In the cases where the objects involved are Parents, I would rather just put

def __div__(self, other):
    return PyNumber_TrueDivide(self, other)

in Parent instead of manually adding __div__ = __truediv__ everywhere.

comment:16 Changed 5 years ago by jdemeyer

I created #18622 to deal with element.pyx

comment:17 Changed 5 years ago by wluebbe

I will work on the other pyx modules with sage-6.8.beta4 (#18622 has just been closed).

comment:18 Changed 5 years ago by git

  • Commit changed from 9701674d59213529ef6476ecc973a8f2cf69bfb3 to a51c0422b3d3a40aaa64ef13329395b840d8ee34

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

a51c042Merge branch 'develop' into u/wluebbe/18578

comment:19 Changed 5 years ago by wluebbe

Resolved merge conflict with sage-6.8.beta5.

comment:20 Changed 5 years ago by jdemeyer

  • Dependencies #18622 deleted

comment:21 follow-up: Changed 5 years ago by jdemeyer

When you continue working on this, please keep in mind 15.

comment:22 Changed 5 years ago by jdemeyer

In src/sage/rings/number_field/number_field_ideal.py, you are replacing _div_ (single underscore!). That's wrong and should not be changed.

comment:23 Changed 5 years ago by git

  • Commit changed from a51c0422b3d3a40aaa64ef13329395b840d8ee34 to 610a2c4123e314979e9895700687aeae79f9e185

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

699cb26Merge branch 'develop' into u/wluebbe/18578
610a2c4Revert error introduced during last merge

comment:24 in reply to: ↑ 21 Changed 5 years ago by wluebbe

Replying to jdemeyer:

When you continue working on this, please keep in mind 15.

Yes, I will.

While merging sage-6.8.beta5 I got confused: I did not notice what your change of number_field_ideal.py in #18622 was. And I pushed before testing the merge ... Then I struggled with undoing my mess :-/

comment:25 Changed 5 years ago by git

  • Commit changed from 610a2c4123e314979e9895700687aeae79f9e185 to 894192195d46cf6b07a43afe0cbc69a54bd3a7f0

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

8941921Trac #18578: Add special function __truediv__() to some pyx-modules

comment:26 Changed 5 years ago by wluebbe

  • Authors set to Wilfried Luebbe
  • Status changed from new to needs_review

comment:27 Changed 5 years ago by wluebbe

Perhaps those tiny new functions might also need doctests?

comment:28 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

See 15

comment:29 Changed 5 years ago by jdemeyer

In src/sage/rings/polynomial/polynomial_element.pyx, in __truediv__, call RingElement.__truediv__ instead RingElement.__div__.

comment:30 Changed 5 years ago by jdemeyer

Don't use

PeriodicRegion.__truediv__(self, other)

in Cython. It's much slower than PyNumber_TrueDivide.

comment:31 Changed 5 years ago by jdemeyer

See also #19535 and #19536 for some partial progress.

comment:32 Changed 5 years ago by jdemeyer

  • Authors changed from Wilfried Luebbe to Wilfried Luebbe, Jeroen Demeyer
  • Milestone changed from sage-6.8 to sage-7.0

comment:33 Changed 5 years ago by jdemeyer

  • Branch changed from u/wluebbe/18578 to u/jdemeyer/18578

comment:34 Changed 5 years ago by jdemeyer

  • Commit changed from 894192195d46cf6b07a43afe0cbc69a54bd3a7f0 to 1d0d01573f63702e802b59633d17ae8dc78bbb98
  • Status changed from needs_work to needs_review

New commits:

1d0d015Trac #18578: Add special function __truediv__() for Py3

comment:35 Changed 5 years ago by jdemeyer

See also #19842.

comment:36 Changed 5 years ago by vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

comment:37 Changed 5 years ago by vbraun

  • Branch changed from u/jdemeyer/18578 to 1d0d01573f63702e802b59633d17ae8dc78bbb98
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.