Opened 21 months ago
Closed 20 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, GitHub, GitLab) | Commit: | b0cdda7aa1636a77fcec7c0b75b32cd187031fec |
Dependencies: | Stopgaps: |
Description (last modified by )
using hash() as suggested in #27886
Change History (25)
comment:1 Changed 21 months ago by
- Branch set to u/chapoton/27972
- Commit set to b0cdda7aa1636a77fcec7c0b75b32cd187031fec
- Description modified (diff)
comment:3 Changed 21 months ago by
- Reviewers set to Vincent Klein
- Status changed from needs_review to positive_review
Looks good to me.
comment:4 Changed 21 months ago by
- Reviewers changed from Vincent Klein to Erik Bray
Was literally in the process of saying the same just now :)
comment:6 Changed 21 months ago by
- Reviewers changed from Vincent Klein to Vincent Klein, Erik Bray
Let's be fair.
comment:7 Changed 21 months ago by
- 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: ↓ 9 ↓ 11 Changed 21 months ago by
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 21 months ago by
comment:10 Changed 21 months ago by
Yes, please. And hopefully someone will review that ticket...
comment:11 in reply to: ↑ 8 Changed 21 months ago by
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 21 months ago by
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 21 months ago by
I already added the patch to #27886.
comment:14 follow-up: ↓ 15 Changed 21 months ago by
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'))
comment:15 in reply to: ↑ 14 Changed 21 months ago by
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: ↓ 17 Changed 21 months ago by
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 21 months ago by
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 21 months ago by
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 21 months ago by
- 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: ↓ 23 Changed 20 months ago by
should we close this one as invalid ?
comment:21 Changed 20 months ago by
Because of #28112?
comment:22 Changed 20 months ago by
rather because of the cython upgrade in #27886, which fixes these doctests in py3
comment:23 in reply to: ↑ 20 Changed 20 months ago by
- 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 20 months ago by
- Status changed from needs_info to positive_review
comment:25 Changed 20 months ago by
- Resolution set to wontfix
- Status changed from positive_review to closed
New commits:
py3: fix last doctests in cpython folder