py3: fix some doctests about hash for real and complex double

[fchapoton]

Component: python3

Author: Frédéric Chapoton

Branch/Commit: b7fd196

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/27344

[fchapoton]

New commits:

f625fd1

py3: fix some doctests about hash for complex and real double

[fchapoton]

Commit: f625fd1

[fchapoton]

Branch: u/chapoton/27344

[jdemeyer]

Reviewer: Jeroen Demeyer

[fchapoton]

comment:3

Thanks a lot, Jeroen. By the way, would you please give your opinion on #27065 ?

[embray]

comment:4

I don't understand these tests. They seem kind of pointless? If we must have doctests for __hash__ methods can they at least prove that they maintain whatever invariant the hash is meant to maintain (which in this case is trivial, but still more meaningful than testing that hash(X) doesn't crash).

[fchapoton]

comment:5

Ahem.. Time lost will never come back.

So, Erik, please tell us exactly what you would like to see as doctests here. Or better, just commit something that suits you and pass on both py2 and py3.

[embray]

comment:6

I'm sorry, I'd be frustrated too, but in the same vein I don't think we should waste time writing pointless doctests that prove nothing.

I personally have no idea what the intent is here for these classes implementing __hash__ or if there's even any reason for them to do so at all. Perhaps we should find that out.

[embray]

comment:7

Just looking at the code I have no reason--it's not documented anywhere--why RDF has to be hashable at all. I'm sure there's a reason, but it would be nice if that were actually explained somewhere. Furthermore I have no idea, even if the class is intended to be used as a singleton, why its hash it set to a (seemingly) arbitrary integer, nor why the old test expected that value to be equivalent to hash(str(RDF)) modulo 2^32. I'd want to know that before just kind of sweeping things under the rug.

[embray]

comment:8

The commit where the original test for RDF was written to its previous form (I'll look at CDF separately) just says "Clean up the fallout from the RDF/CDF hashing patch" which doesn't really answer anything.

In particular I don't get the "mod 2!^32" part, although it does appear to hold accurate on Python 2. Though it seems part of the point is that hash(RDF) and hash(str(RDF)) are not the same:

sage: {RDF: 1, str(RDF): 2}
{Real Double Field: 1, 'Real Double Field': 2}

ISTM part of the point of giving it a fixed value was at some point primarily for some performance purpose, but it's not clear what that purpose was exactly or what benchmark this was targeting.

[jdemeyer]

comment:9

It's clear to me that the only thing that matters is that hash(RDF) works and that it gives a constant (within one Python session at least) value. So there is nothing wrong with return 561162115 as implementation.

[embray]

comment:10

Replying to @embray:

In particular I don't get the "mod 2!^32" part

I think the reason for this, at least, was that on 32-bit Python the str hashing function overflows, whereas on 64-bit it isn't as likely to overflow for a short string, but either way between 32-bit and 64-bit the hash values wind up the same modulo 2^!32.

That still raises the question of why there was a desire for hash(RDF) == hash(str(RDF)) especially since that does not hold on 64-bit.

[embray]

comment:11

Replying to @jdemeyer:

It's clear to me that the only thing that matters is that hash(RDF) works and that it gives a constant (within one Python session at least) value. So there is nothing wrong with return 561162115 as implementation.

I think then it suffices to demonstrate that hash(RDF) == hash(RealDoubleField_class()) and to mention that this class is intended for use as a singleton so any instance of it should be equivalent from a hashing perspective.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

9cc1513

better hash tests

[sagetrac-git]

Changed commit from f625fd1 to 9cc1513

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b7fd196

py3: fix some doctests about hash for complex and real double

[sagetrac-git]

Changed commit from 9cc1513 to b7fd196

[fchapoton]

comment:14

like that ?

[vbraun]

Changed branch from u/chapoton/27344 to b7fd196