Opened 2 years ago

Closed 2 years ago

#27972 closed enhancement (wontfix)

py3: fix last doctest in cpython folder

Reported by: chapoton Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: python3 Keywords:
Cc: jhpalmieri, tscrim Merged in:
Authors: Frédéric Chapoton Reviewers: Vincent Klein, Erik Bray
Report Upstream: N/A Work issues:
Branch: u/chapoton/27972 (Commits, GitHub, GitLab) Commit: b0cdda7aa1636a77fcec7c0b75b32cd187031fec
Dependencies: Stopgaps:

Status badges

Description (last modified by chapoton)

using hash() as suggested in #27886

Change History (25)

comment:1 Changed 2 years ago by chapoton

  • Branch set to u/chapoton/27972
  • Commit set to b0cdda7aa1636a77fcec7c0b75b32cd187031fec
  • Description modified (diff)

New commits:

b0cdda7py3: fix last doctests in cpython folder

comment:2 Changed 2 years ago by chapoton

  • Status changed from new to needs_review

an easy one

comment:3 Changed 2 years ago by vklein

  • Reviewers set to Vincent Klein
  • Status changed from needs_review to positive_review

Looks good to me.

comment:4 Changed 2 years ago by embray

  • Reviewers changed from Vincent Klein to Erik Bray

Was literally in the process of saying the same just now :)

comment:5 Changed 2 years ago by embray

  • Reviewers changed from Erik Bray to Vincent Klein

Oops!

comment:6 Changed 2 years ago by vklein

  • Reviewers changed from Vincent Klein to Vincent Klein, Erik Bray

Let's be fair.

comment:7 Changed 2 years ago by jdemeyer

  • Status changed from positive_review to needs_info

There is nothing wrong with the Sage code, it should work. It would be better to just apply the upstream fix https://github.com/cython/cython/commit/28251032f86c266065e4976080230481b1a1bb29 instead.

comment:8 follow-ups: Changed 2 years ago by chapoton

Better maybe, but not for the goal of having all tests pass soon in python3.

Jeroen, will you be able to provide your alternative solution as a branch ? You seem to have less activity on trac, so I guess you have more urgent matters to handle.

Otherwise, maybe this could be done in another ticket ?

comment:9 in reply to: ↑ 8 Changed 2 years ago by jdemeyer

Replying to chapoton:

Jeroen, will you be able to provide your alternative solution as a branch ?

Should I add it to #27886? It should take little effort.

comment:10 Changed 2 years ago by chapoton

Yes, please. And hopefully someone will review that ticket...

comment:11 in reply to: ↑ 8 Changed 2 years ago by jdemeyer

Replying to chapoton:

You seem to have less activity on trac, so I guess you have more urgent matters to handle.

Not really "more urgent matters" but more "other matters".

In case you are wondering what I have been doing recently: https://docs.python.org/3.8/whatsnew/3.8.html#vectorcall-a-fast-calling-protocol-for-cpython

comment:12 Changed 2 years ago by embray

There is a very simple workaround here, that makes logical sense for the tests (passing an object through hash() necessarily returns whatever the acceptable type for a hash is). Let's just fix this without having to rely yet again on patching cython or whatever.

comment:13 Changed 2 years ago by jdemeyer

I already added the patch to #27886.

comment:14 follow-up: Changed 2 years ago by jhpalmieri

I feel like the changes here make sense independently of the Cython patch: I believe that

sage: D = {1230981308409180310928308123: ZZ} # random large integer
sage: test_del_dictitem_by_exact_value(D, ZZ, 1230981308409180310928308123)

will fail, while

sage: D = {1230981308409180310928308123: ZZ}
sage: test_del_dictitem_by_exact_value(D, ZZ, hash(1230981308409180310928308123))

will work.

I don't object to testing whether small integers are equal to their hash values, but that is tested in integer.pyx. After the changes here, the doctest still tests the appropriate function, and they also indicate how to use the function:

sage: D = {'a': ZZ}
sage: test_del_dictitem_by_exact_value(D, ZZ, hash('a'))
Last edited 2 years ago by jhpalmieri (previous) (diff)

comment:15 in reply to: ↑ 14 Changed 2 years ago by jdemeyer

Replying to jhpalmieri:

I don't object to testing whether small integers are equal to their hash values, but that is tested in integer.pyx.

True, but unrelated to this ticket. That's not what this doctest is testing.

comment:16 follow-up: Changed 2 years ago by jhpalmieri

Are you agreeing with me or disagreeing? I can't tell.

What is the doctest supposed to be testing?

comment:17 in reply to: ↑ 16 Changed 2 years ago by jdemeyer

Replying to jhpalmieri:

What is the doctest supposed to be testing?

That nothing happens if you give the wrong hash value. So basically the test relies on hash(1) != 2.

comment:18 Changed 2 years ago by jhpalmieri

The second doctest relies on hash(1) == 1. Well, it doesn't rely on it, but if hash(1) != 1, then I think it is not testing what is intended.

And of course both rely on Sage Integers being valid hash values, which is what is fixed by the patch you posted to #27886.

comment:19 Changed 2 years ago by embray

  • Milestone changed from sage-8.8 to sage-8.9

Tickets still needing working or clarification should be moved to the next release milestone at the soonest (please feel free to revert if you think the ticket is close to being resolved).

comment:20 follow-up: Changed 2 years ago by chapoton

should we close this one as invalid ?

comment:21 Changed 2 years ago by jhpalmieri

Because of #28112?

comment:22 Changed 2 years ago by chapoton

rather because of the cython upgrade in #27886, which fixes these doctests in py3

comment:23 in reply to: ↑ 20 Changed 2 years ago by vklein

  • Milestone changed from sage-8.9 to sage-duplicate/invalid/wontfix

Replying to chapoton:

should we close this one as invalid ?

Sure. All cpython's module tests pass with 8.9.beta1/py3.

comment:24 Changed 2 years ago by vklein

  • Status changed from needs_info to positive_review

comment:25 Changed 2 years ago by chapoton

  • Resolution set to wontfix
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.