Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wluebbe mannequin opened this issue Jun 2, 2015 · 45 comments
Closed

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

wluebbe mannequin opened this issue Jun 2, 2015 · 45 comments

Comments

@wluebbe
Copy link
Mannequin

wluebbe mannequin commented Jun 2, 2015

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.

Component: misc

Author: Wilfried Luebbe, Jeroen Demeyer

Branch/Commit: 1d0d015

Reviewer: Volker Braun

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

@wluebbe wluebbe mannequin added this to the sage-6.8 milestone Jun 2, 2015
@wluebbe wluebbe mannequin added p: major / 3 labels Jun 2, 2015
@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Jun 2, 2015

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

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Jun 2, 2015

comment:1

Attachment: test__div__.py.gz

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)

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Jun 2, 2015

Attachment: grep-of-div__.txt

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

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Jun 2, 2015

comment:2

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

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Jun 2, 2015

Branch: u/wluebbe/18578

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Jun 2, 2015

Commit: 9701674

@wluebbe wluebbe mannequin self-assigned this Jun 2, 2015
@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Jun 2, 2015

comment:3

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__().

@jdemeyer
Copy link

jdemeyer commented Jun 2, 2015

comment:4

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.

@jdemeyer
Copy link

jdemeyer commented Jun 2, 2015

comment:5

How about this in Cython?

from cpython.number cimport PyNumber_TrueDivide

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

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Jun 2, 2015

comment:6

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!

@jdemeyer
Copy link

jdemeyer commented Jun 2, 2015

comment:7

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

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Jun 2, 2015

comment:8

Yes, that did the trick.

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

Thanks for the help!

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Jun 2, 2015

comment:9

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):?

@jdemeyer
Copy link

jdemeyer commented Jun 3, 2015

comment:10

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

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Jun 3, 2015

comment:11

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

@jdemeyer
Copy link

jdemeyer commented Jun 3, 2015

comment:12

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)

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Jun 3, 2015

comment:13

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

@jdemeyer
Copy link

jdemeyer commented Jun 6, 2015

Dependencies: #18622

@jdemeyer
Copy link

jdemeyer commented Jun 6, 2015

comment:15

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.

@jdemeyer
Copy link

jdemeyer commented Jun 6, 2015

comment:16

I created #18622 to deal with element.pyx

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Jun 9, 2015

comment:17

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 22, 2015

Changed commit from 9701674 to a51c042

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 22, 2015

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

a51c042Merge branch 'develop' into u/wluebbe/18578

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Jun 22, 2015

comment:19

Resolved merge conflict with sage-6.8.beta5.

@jdemeyer
Copy link

Changed dependencies from #18622 to none

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 22, 2015

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

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

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Jun 22, 2015

comment:24

Replying to @jdemeyer:

When you continue working on this, please keep in mind [comment: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 :-/

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 22, 2015

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 22, 2015

Changed commit from 610a2c4 to 8941921

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Jun 22, 2015

Author: Wilfried Luebbe

@wluebbe wluebbe mannequin added the s: needs review label Jun 22, 2015
@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Jun 22, 2015

comment:27

Perhaps those tiny new functions might also need doctests?

@jdemeyer
Copy link

comment:28

See [comment:15]

@jdemeyer
Copy link

comment:29

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

@jdemeyer
Copy link

comment:30

Don't use

PeriodicRegion.__truediv__(self, other)

in Cython. It's much slower than PyNumber_TrueDivide.

@jdemeyer
Copy link

jdemeyer commented Nov 6, 2015

comment:31

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

@jdemeyer
Copy link

jdemeyer commented Jan 7, 2016

Changed author from Wilfried Luebbe to Wilfried Luebbe, Jeroen Demeyer

@jdemeyer jdemeyer modified the milestones: sage-6.8, sage-7.0 Jan 7, 2016
@jdemeyer
Copy link

jdemeyer commented Jan 7, 2016

Changed branch from u/wluebbe/18578 to u/jdemeyer/18578

@jdemeyer
Copy link

jdemeyer commented Jan 7, 2016

New commits:

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

@jdemeyer
Copy link

jdemeyer commented Jan 7, 2016

Changed commit from 8941921 to 1d0d015

@jdemeyer
Copy link

jdemeyer commented Jan 7, 2016

comment:35

See also #19842.

@vbraun
Copy link
Member

vbraun commented Jan 7, 2016

Reviewer: Volker Braun

@vbraun
Copy link
Member

vbraun commented Jan 8, 2016

Changed branch from u/jdemeyer/18578 to 1d0d015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants