#26855 closed enhancement (fixed)
py3: fixes for doctests in cpython
| Reported by: | chapoton | Owned by: | |
|---|---|---|---|
| Priority: | major | Milestone: | sage-8.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: |
Status badges
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 follow-up: ↓ 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 plain-Python __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/site-packages/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/site-packages/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 follow-up: ↓ 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->intwas 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 sage-8.5 to sage-8.6
This tickets were closed as fixed after the Sage 8.5 release.
New commits:
py3: fixes for doctests of cpython