#24805 closed defect (fixed)

py3: miscellaneous division-related fixes, particularly for sage_setup.autogen

Reported by: embray Owned by:
Priority: major Milestone: sage-8.2
Component: python3 Keywords: division
Cc: Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 28e567d (Commits) Commit: 28e567d1ea2fb27377df4b2db51f1c59ca1fbd79
Dependencies: Stopgaps:

Description

Previously we used an opcode named 'div' both for the old div operator and for truediv (for compatibility's sake, since use of __truediv__) depends on whether or not from __future__ import division is in effect).

So again, for compatibility's sake, on Python 3 we call truediv just 'div', while integer division is still explicitly 'floordiv'.

Includes a few other semi-related fixes found while testing. This fixes most but not all Python 3 issues with sage.ext.fast_callable--at least those directly related to division.

Change History (23)

comment:1 Changed 15 months ago by git

  • Commit set to b1d93d363909303fe1d7ad5e6d648734ad753fe9

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

b1d93d3py3: some fixes related to properly handling division, particularly in sage_setup.autogen

comment:2 Changed 15 months ago by git

  • Commit changed from b1d93d363909303fe1d7ad5e6d648734ad753fe9 to 7d1a12772dc957b9178a2060c35d2c7b32d137d6

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

7d1a127py3: some fixes related to properly handling division, particularly in sage_setup.autogen

comment:3 Changed 15 months ago by git

  • Commit changed from 7d1a12772dc957b9178a2060c35d2c7b32d137d6 to 22b8f90baf7631619b71c278df311276f0c3bb80

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

22b8f90fix to these tests to make them a little more flexible

comment:4 Changed 15 months ago by embray

  • Status changed from new to needs_review

comment:5 Changed 15 months ago by jdemeyer

Wouldn't it make sense to use PyNumber_TrueDivide even on Python 2?

comment:6 follow-up: Changed 15 months ago by embray

That's a good question. Do we want to just always assume Python 3 division semantics?

comment:7 in reply to: ↑ 6 Changed 15 months ago by jdemeyer

Replying to embray:

That's a good question. Do we want to just always assume Python 3 division semantics?

Sage typically assumes true division semantics anyway. For example, division of Integer instances is closer to int.__truediv__ than to int.__div__. So, with this in mind, I would indeed use Python 3 division everywhere (assuming that it doesn't break tests of course).

comment:8 follow-up: Changed 15 months ago by embray

I'm fine with that, though I've found at least a few classes that implement __div__ but not __truediv__. It would also in principle break user-defined classes, if any were actually used in this context, that implement __div__ but not __truediv__.

Perhaps instead of using PyNumber_(True)Divide it could use a little wrapper macro which prefers __truediv__ if it exists, or falls back on __div__?

comment:9 in reply to: ↑ 8 ; follow-up: Changed 15 months ago by jdemeyer

Replying to embray:

I'm fine with that, though I've found at least a few classes that implement __div__ but not __truediv__.

Those should obviously be fixed. But keep in mind that everything inheriting from Element already supports both __truediv__ and __div__ through the coercion model.

Perhaps instead of using PyNumber_(True)Divide it could use a little wrapper macro which prefers __truediv__ if it exists, or falls back on __div__?

I'm not against that, but I also don't think that it's important. If you do, I guess you should output a deprecation warning if __div__ is used.

comment:10 in reply to: ↑ 9 Changed 15 months ago by embray

Replying to jdemeyer:

Replying to embray:

I'm fine with that, though I've found at least a few classes that implement __div__ but not __truediv__.

Those should obviously be fixed. But keep in mind that everything inheriting from Element already supports both __truediv__ and __div__ through the coercion model.

Yep.

Perhaps instead of using PyNumber_(True)Divide it could use a little wrapper macro which prefers __truediv__ if it exists, or falls back on __div__?

I'm not against that, but I also don't think that it's important.

I agree it's probably not a very likely scenario, but I did it anyways.

If you do, I guess you should output a deprecation warning if __div__ is used.

Yep, did that.

comment:11 Changed 15 months ago by git

  • Commit changed from 22b8f90baf7631619b71c278df311276f0c3bb80 to cc10abab95f97f09de3e7e5c7107d346e6273196

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

cc10abaInstead of always using PyNumber_Divide on Python 3, go through a helper

comment:12 Changed 15 months ago by embray

Perhaps something like this.

comment:13 Changed 15 months ago by embray

  • Keywords division added

comment:14 Changed 15 months ago by jdemeyer

  • Status changed from needs_review to needs_work

The way it's written now, it looks like the deprecation warning will be shown every time that __truediv__ fails with a TypeError, regardless of whether __div__ works.

Maybe better something like:

try:
    ...
except TypeError:
    IF PY_MAJOR_VERSION < 3:
        res = PyNumber_Divide(...)
        deprecation(...)
        return res
    ELSE
        raise
    ENDIF

This way, the deprecation will only be shown if __div__ actually worked.

comment:15 Changed 15 months ago by embray

Yes, I was thinking of doing something like that as well, but I couldn't quite decide on the logic for it. But I think your suggestion makes sense.

comment:16 Changed 15 months ago by git

  • Commit changed from cc10abab95f97f09de3e7e5c7107d346e6273196 to 28e567d1ea2fb27377df4b2db51f1c59ca1fbd79

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

28e567dMake sure the deprecation warning is only shown if __div__ actually worked.

comment:17 Changed 15 months ago by embray

  • Status changed from needs_work to needs_review

comment:18 Changed 15 months ago by jdemeyer

I am getting consistent doctest timeouts on a particular test system:

sage -t src/sage/plot/plot3d/list_plot3d.py  # Timed out
sage -t src/sage/geometry/riemannian_manifolds/surface3d_generators.py  # Timed out
sage -t src/sage/stats/distributions/discrete_gaussian_lattice.py  # Timed out

I'll need to check whether this ticket has something to do with that.

comment:19 Changed 15 months ago by jdemeyer

The timeouts are unrelated to this ticket. They are still disturbing though...

comment:20 Changed 15 months ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:21 Changed 15 months ago by embray

I've been getting some timeouts too sometimes, but I'm not sure if it's the same modules. I'll check.

comment:22 Changed 15 months ago by jdemeyer

Sorry, forgot to say... this is #13135.

comment:23 Changed 15 months ago by vbraun

  • Branch changed from u/embray/python3/division-misc to 28e567d1ea2fb27377df4b2db51f1c59ca1fbd79
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.