Add Py_HashDouble() function

[vstinner]

• PR: gh-111545: Add Py_HashDouble() function python/cpython#112449

• API: int Py_HashDouble(double value, Py_hash_t *result)

  • Set *result to the hash value and return 1 on success.
  • Set *result to 0 and return 0 if the hash value cannot be calculated. For example, if value is not-a-number (NaN).
  • result must not be NULL

• Stable ABI: compatible

• Not added to the limited C API yet

UPDATE: I updated the API documation to address @zooba's suggestion (make the API more generic, be less specific about NaN case).

Voters:

• Guido Van Rossum @gvanrossum
• Irit Katriel @iritkatriel
• Steve Dower @zooba
• Petr Viktorin @encukou

Since I proposed this API and I'm part of the C API Working Group, I will not vote on this decision.

[zooba]

I'm unclear why this needs to be a public API rather than going through a PyObject. What can you do with a primitive hash value for a primitive value?

Assuming that it does need a dedicated, public API, I prefer the return values as described in this post (and not the other variants that were tried), though I would prefer if they were described as "returns 1 if result is valid; otherwise 0" and says elsewhere that NaN cannot be hashed. It should also clearly say that it does not set a Python exception (and if it did, then I think the result should be negative).

[vstinner]

I'm unclear why this needs to be a public API rather than going through a PyObject

The rationale is explained in the issue. numpy currently uses the _Py_HashDouble() function: numpy/numpy#25035

float_arrtype_hash() calls Npy_HashDouble() with (double)PyArrayScalar_VAL(obj, @name@) value:

https://github.com/numpy/numpy/blob/742920aef552b27806094f8ed6462d3cf1744785/numpy/_core/src/multiarray/scalartypes.c.src#L3629-L3633

I don't see how existing PyObject_Hash() API can be used for that.

Assuming that it does need a dedicated, public API, I prefer the return values as described in this post (and not the other variants that were tried), though I would prefer if they were described as "returns 1 if result is valid; otherwise 0" and says elsewhere that NaN cannot be hashed.

Isn't the proposed API?

Technically, the function returns 0 for not-a-number and it can be used as the hash value for some use cases. Hash conflict can lead to inefficient hash table but it's usually correct. If you want to mimick Python hash(float), you should call Py_HashPointer(obj) if the C double is not-a-number.

It should also clearly say that it does not set a Python exception (and if it did, then I think the result should be negative).

The function cannot fail: it cannot return 0 and it cannot set an exception. It can fix the doc to make it more explicit if needed.

[zooba]

numpy currently uses the _Py_HashDouble() function

So the rationale is: "libraries that implement their own numeric type that needs to compare equal to the built-in float type may use this to implement their tp_hash function".

I don't see how existing PyObject_Hash() API can be used for that.

The object_arrtype_hash function a few lines below your link does it. If you need an identical hash to an existing Python type, then you convert the value to that type and then call PyObject_Hash.

Isn't the proposed API?

Yes, but the way you describe it isn't a good way to document it. The return value should reflect the validity of the result or post conditions, rather than the values of the arguments. That way we retain freedom to change the reasons it may fail without having to change the documentation (and follow a deprecation process).

It's the difference between:

• "returns 0 when you pass NaN" (bad - too specific)
• "returns 0 when it couldn't calculate a hash value" (good)

And in the latter case, we can also document that we can't calculate hash values for NaN. But that's disconnected from the definition of the result, so if it one day turns out we can't calculate a hash for some other value then we can start returning an error without having to create a new function. That may be unlikely in this particular case, but the general principle applies elsewhere, and the docs here should be consistent with the other places where it is more likely that over-specifying the function could get us into trouble.

Again, it's just grammar. But in a specification, which is how our documentation is used, the grammar matters.

I can fix the doc to make it more explicit if needed.

Yes. Fixing the doc is what I was asking for. Don't assume that users will read our source code.

[vstinner]

"returns 0 when it couldn't calculate a hash value" (good)

In Python <= 3.9, hash(float("nan")) was just 0. It's not as if Python "cannot calculate" a hash value. Or that the hash value 0 will put your computer on fire. When proposed API return 0, it really means "not-a-number": as a hint that you might want to use a different hash value if you want.

In Python <= 3.9, all NaN float had a same hash value, even if they were different Python objects:

$ python3.9
Python 3.9.18 (main, Aug 28 2023, 00:00:00) 
>>> import math
>>> x=math.inf-math.inf
>>> y=math.inf-math.inf
>>> x is y
False
>>> hash(x), hash(y)
(0, 0)

So if these Python float objects are used as dict keys for example, hash collisions were more likely. The Python dict type is fine with hash collision, since NaN is not equal to NaN:

>>> x == y
False
>>> d={x: 1, y: 2}
>>> len(d)
2
>>> d[x], d[y]
(1, 2)
>>> d
{nan: 1, nan: 2}

Python 3.10 modified hash(float) by using object.__hash__(obj) for NaN floats to reduce hash collisions.

But maybe for some other usages, you might want to use something else.

There are 3 categories of float values:

• finite: return (more or less) hash(int(value)) (in an optimized way) -- it can be equal to 0. IEEE 754 sub-normals values fall into this category as well.
• infinite: return sys.hash_info.inf or -sys.hash_info.inf.
• NaN: return 0.

Note: maybe we should have this discussion on python/cpython#112449 PR or python/cpython#111545 issue.

[vstinner]

If proposed API is confusing, an alternative is Py_hash_t Py_HashDouble(double value, int *is_nan). It might be less efficient if we accept NULL for is_nan. But such API may be less confusing.

Or it can be just Py_hash_t Py_HashDouble(double value) and the caller would have to call isnan(value) explicitly. It's less efficient, since Py_HashDouble() already calls isnan(value).

[zooba]

The proposed API isn't confusing, I just disagree with how you describe it.

We don't say "if <parameter> contains a specific value, the return value is 0". We say "if the result cannot be calculated, the return value is 0. One reason the result may not be calculated is if <parameter> contains a specific value."

Do you see the difference between those two statements?

[vstinner]

The proposed API isn't confusing, I just disagree with how you describe it.

I thought that the vote was restricted on the API.

If you have concerns on the implementation (doc, tests, whatever), I just suggest to move the discussion on the PR instead. I'm sure that the doc can be enhanced :-)

[zooba]

I have a concern with your API design.

That concern is that you designed: "when the input is NaN, returns 0".

I can tell this is your design because of the documentation.

I think your design should be: "when it fails to generate a hash, returns 0".

I'll be able to tell that you've fixed the design when the documentation is changed.

This is all about API design. The documentation you've written is how I can tell what you've designed.

Once it's released, we can't just change the documentation, because it will change the definition of the API in a subtly incompatible way. It doesn't affect the current implementation, but it could in the future, so I want to address it now so that we don't run into issues in the future.

Please don't treat this process as just a checkbox before you do whatever you want. We're trying to collaborate to produce a consistent, reliable, forward-compatible API that doesn't get us into trouble later on. If you think I'm just arbitrarily impeding you, feel free to make a complaint to the SC and get me removed from the WG - we have a process for this now.

[vstinner]

Well, I was thinking aloud since I'm not used to the C API Working Group. In short, I proposed to update the PR to address your concern, and then come back to the decision. I didn't propose to make a decision and change the API afterwards.

I can tell this is your design because of the documentation.

It's kind to refer to this API as "my design", but it is not mine 😁. It was a collective work on issue python/cpython#111545. I only tried to lead the work to have a full change (implementation, doc, tests) addressing all concerns. I prefer to rely on other IEEE 754 experts as Mark Dickinshon who is the latest author who authored a major redesign of all hash() functions for "numbers (int, float, complex, Decimal) to have consistent hash values for all stdlib number types.

My concern is more that the PR was already approved twice, and now we are discussing further changes. It's a new situation to me, I don't know if the C API Working Group should take a decision, and then the PR is updated. Or the opposite: the WG suggests change, the PR is updated and maybe I wait to new approval, and then the WG makes a decision.

---

That being said, I applied your documentation suggestion in the description of this issue: #2 (comment) I'm fine with it, it's reasonable to make the API more generic, and less specific about "NaN" (only mention it as "one possible case" of "cannot calculate the hash value").

First, I expected "the API" to be int Py_HashDouble(double value, Py_hash_t *result) and nothing else. But I agree that the description of the parameters, output, error cases, etc. are part of the API. Well, that's also why I added it to this issue :-) So the API is the function definition and its documentation.

[zooba]

With the clarified return value semantics, I'm happy with this design.

[vstinner]

@gvanrossum, @iritkatriel, @encukou: so, what do you think of this API? Tell me if you need more details.

[iritkatriel]

LGTM. I left some comments on documentation on the PR.

[gvanrossum]

I am not reviewing the PR (no time) but I am okay with the name and spec of the new API, given that there is a real need (e.g. Numpy).