Opened 5 months ago

Closed 4 months 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) Commit: b0cdda7aa1636a77fcec7c0b75b32cd187031fec
Dependencies: Stopgaps:

Description (last modified by chapoton)

using hash() as suggested in #27886

Change History (25)

comment:1 Changed 5 months 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 5 months ago by chapoton

  • Status changed from new to needs_review

an easy one

comment:3 Changed 5 months ago by vklein

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

Looks good to me.

comment:4 Changed 5 months 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 5 months ago by embray

  • Reviewers changed from Erik Bray to Vincent Klein

Oops!

comment:6 Changed 5 months ago by vklein

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

Let's be fair.

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

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

comment:11 in reply to: ↑ 8 Changed 5 months 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 5 months 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 5 months ago by jdemeyer

I already added the patch to #27886.

comment:14 follow-up: Changed 5 months 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 5 months ago by jhpalmieri (previous) (diff)

comment:15 in reply to: ↑ 14 Changed 5 months 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 5 months 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 5 months 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 5 months 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 5 months 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 4 months ago by chapoton

should we close this one as invalid ?

comment:21 Changed 4 months ago by jhpalmieri

Because of #28112?

comment:22 Changed 4 months ago by chapoton

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

comment:23 in reply to: ↑ 20 Changed 4 months 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 4 months ago by vklein

  • Status changed from needs_info to positive_review

comment:25 Changed 4 months ago by chapoton

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