Opened 2 years ago
Closed 2 years ago
#24805 closed defect (fixed)
py3: miscellaneous divisionrelated fixes, particularly for sage_setup.autogen
Reported by:  embray  Owned by:  

Priority:  major  Milestone:  sage8.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 semirelated 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 2 years ago by
 Commit set to b1d93d363909303fe1d7ad5e6d648734ad753fe9
comment:2 Changed 2 years ago by
 Commit changed from b1d93d363909303fe1d7ad5e6d648734ad753fe9 to 7d1a12772dc957b9178a2060c35d2c7b32d137d6
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
7d1a127  py3: some fixes related to properly handling division, particularly in sage_setup.autogen

comment:3 Changed 2 years ago by
 Commit changed from 7d1a12772dc957b9178a2060c35d2c7b32d137d6 to 22b8f90baf7631619b71c278df311276f0c3bb80
Branch pushed to git repo; I updated commit sha1. New commits:
22b8f90  fix to these tests to make them a little more flexible

comment:4 Changed 2 years ago by
 Status changed from new to needs_review
comment:5 Changed 2 years ago by
Wouldn't it make sense to use PyNumber_TrueDivide
even on Python 2?
comment:6 followup: ↓ 7 Changed 2 years ago by
That's a good question. Do we want to just always assume Python 3 division semantics?
comment:7 in reply to: ↑ 6 Changed 2 years ago by
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 followup: ↓ 9 Changed 2 years ago by
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 userdefined 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 ; followup: ↓ 10 Changed 2 years ago by
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 2 years ago by
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 2 years ago by
 Commit changed from 22b8f90baf7631619b71c278df311276f0c3bb80 to cc10abab95f97f09de3e7e5c7107d346e6273196
Branch pushed to git repo; I updated commit sha1. New commits:
cc10aba  Instead of always using PyNumber_Divide on Python 3, go through a helper

comment:12 Changed 2 years ago by
Perhaps something like this.
comment:13 Changed 2 years ago by
 Keywords division added
comment:14 Changed 2 years ago by
 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 2 years ago by
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 2 years ago by
 Commit changed from cc10abab95f97f09de3e7e5c7107d346e6273196 to 28e567d1ea2fb27377df4b2db51f1c59ca1fbd79
Branch pushed to git repo; I updated commit sha1. New commits:
28e567d  Make sure the deprecation warning is only shown if __div__ actually worked.

comment:17 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:18 Changed 2 years ago by
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 2 years ago by
The timeouts are unrelated to this ticket. They are still disturbing though...
comment:20 Changed 2 years ago by
 Reviewers set to Jeroen Demeyer
 Status changed from needs_review to positive_review
comment:21 Changed 2 years ago by
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 2 years ago by
Sorry, forgot to say... this is #13135.
comment:23 Changed 2 years ago by
 Branch changed from u/embray/python3/divisionmisc to 28e567d1ea2fb27377df4b2db51f1c59ca1fbd79
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
py3: some fixes related to properly handling division, particularly in sage_setup.autogen