#26855 closed enhancement (fixed)
py3: fixes for doctests in cpython
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage8.6 
Component:  python3  Keywords:  
Cc:  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Travis Scrimshaw 
Report Upstream:  Fixed upstream, but not in a stable release.  Work issues:  
Branch:  8dde842 (Commits, GitHub, GitLab)  Commit:  8dde84265aa045c38b02ac8a3380e84ec99822ff 
Dependencies:  Stopgaps: 
Description (last modified by )
Reported upstream: https://github.com/cython/cython/issues/2752
Change History (16)
comment:1 Changed 2 years ago by
 Branch set to u/chapoton/26855
 Commit set to fb4d072e546dd2a9da2c76d0340d85b824f92067
 Status changed from new to needs_review
comment:2 Changed 2 years ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
LGTM.
comment:3 Changed 2 years ago by
 Status changed from positive_review to needs_info
Why is this needed?
 sage: test_del_dictitem_by_exact_value(D, ZZ, 2) + sage: test_del_dictitem_by_exact_value(D, ZZ, int(2))
If this change is really needed, I consider it a bug in Cython.
comment:4 Changed 2 years ago by
General rule about fixing Python 3 doctests: don't "fix" the doctest but fix the code such that the doctest remains working.
comment:5 followup: ↓ 6 Changed 2 years ago by
because in python 3, the hash must be a python int...
comment:6 in reply to: ↑ 5 Changed 2 years ago by
Replying to chapoton:
because in python 3, the hash must be a python int...
What do you mean? Are you talking about the output of a plainPython __hash__
method? This is not involved here.
comment:7 Changed 2 years ago by
sage t long src/sage/cpython/dict_del_by_value.pyx ********************************************************************** File "src/sage/cpython/dict_del_by_value.pyx", line 435, in sage.cpython.dict_del_by_value.test_del_dictitem_by_exact_value Failed example: test_del_dictitem_by_exact_value(D, ZZ, 2) Exception raised: Traceback (most recent call last): File "/home/u1/chapoton/sage3/local/lib/python3.6/sitepackages/sage/doctest/forker.py", line 671, in _run self.compile_and_execute(example, compiler, test.globs) File "/home/u1/chapoton/sage3/local/lib/python3.6/sitepackages/sage/doctest/forker.py", line 1086, in compile_and_execute exec(compiled, globs) File "<doctest sage.cpython.dict_del_by_value.test_del_dictitem_by_exact_value[7]>", line 1, in <module> test_del_dictitem_by_exact_value(D, ZZ, Integer(2)) File "sage/cpython/dict_del_by_value.pyx", line 443, in sage.cpython.dict_del_by_value.test_del_dictitem_by_exact_value (build/cythonized/sage/cpython/dict_del_by_value.c:2504) return del_dictitem_by_exact_value(<PyDictObject *>D, <PyObject *>value, h) TypeError: an integer is required
comment:8 Changed 2 years ago by
I'm assuming that this is an exception raised by Cython. So I would like to understand why it works on Python 2 but not on Python 3. This is not only going to be relevant for this ticket, but possibly for many other Python 3 problems regarding Sage Integer
vs Python int
.
comment:9 followup: ↓ 10 Changed 2 years ago by
My thought was that the coercion (conversion?) from Integer
> int
was somehow not done automatically in Python3 but it was allowed in Python2.
comment:10 in reply to: ↑ 9 Changed 2 years ago by
Replying to tscrim:
My thought was that the coercion (conversion?) from
Integer
>int
was somehow not done automatically in Python3 but it was allowed in Python2.
I think it's at least something that we should investigate in general.
comment:11 Changed 2 years ago by
Possibly we are doing something wrong in integer.pyx
...
comment:12 Changed 2 years ago by
 Description modified (diff)
 Report Upstream changed from N/A to Reported upstream. No feedback yet.
comment:13 Changed 2 years ago by
 Branch changed from u/chapoton/26855 to u/jdemeyer/26855
comment:14 Changed 2 years ago by
 Commit changed from fb4d072e546dd2a9da2c76d0340d85b824f92067 to 8dde84265aa045c38b02ac8a3380e84ec99822ff
 Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release.
 Status changed from needs_info to positive_review
The problem with Py_hash_t
should be fixed upstream in Cython. So I will revert this part of this ticket, which should be solved whenever we upgrade Cython.
New commits:
8dde842  py3: fixes for doctests of cpython

comment:15 Changed 2 years ago by
 Branch changed from u/jdemeyer/26855 to 8dde84265aa045c38b02ac8a3380e84ec99822ff
 Resolution set to fixed
 Status changed from positive_review to closed
comment:16 Changed 2 years ago by
 Milestone changed from sage8.5 to sage8.6
This tickets were closed as fixed after the Sage 8.5 release.
New commits:
py3: fixes for doctests of cpython