Opened 6 months ago

Closed 5 months ago

Last modified 5 months ago

#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) Commit: 8dde84265aa045c38b02ac8a3380e84ec99822ff
Dependencies: Stopgaps:

Change History (16)

comment:1 Changed 6 months ago by chapoton

  • Branch set to u/chapoton/26855
  • Commit set to fb4d072e546dd2a9da2c76d0340d85b824f92067
  • Status changed from new to needs_review

New commits:

fb4d072py3: fixes for doctests of cpython

comment:2 Changed 6 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM.

comment:3 Changed 6 months ago by jdemeyer

  • 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 6 months ago by jdemeyer

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: Changed 6 months ago by chapoton

because in python 3, the hash must be a python int...

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

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.

Last edited 6 months ago by jdemeyer (previous) (diff)

comment:7 Changed 6 months ago by chapoton

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 6 months ago by jdemeyer

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: Changed 6 months ago by tscrim

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 6 months ago by jdemeyer

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 6 months ago by jdemeyer

Possibly we are doing something wrong in integer.pyx...

comment:12 Changed 6 months ago by jdemeyer

  • Description modified (diff)
  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

comment:13 Changed 6 months ago by jdemeyer

  • Branch changed from u/chapoton/26855 to u/jdemeyer/26855

comment:14 Changed 6 months ago by jdemeyer

  • 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:

8dde842py3: fixes for doctests of cpython

comment:15 Changed 5 months ago by vbraun

  • Branch changed from u/jdemeyer/26855 to 8dde84265aa045c38b02ac8a3380e84ec99822ff
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:16 Changed 5 months ago by embray

  • Milestone changed from sage-8.5 to sage-8.6

This tickets were closed as fixed after the Sage 8.5 release.

Note: See TracTickets for help on using tickets.