Add Py_HashDouble() function

@zooba

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.

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).

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.

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.

"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.

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).

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?

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 :-)

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.

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.

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

@gvanrossum

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

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

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).

LGTM too; the new documentation is much better. Thank you!

We agreed on int Py_HashDouble(double value, Py_hash_t *result), but Serhiy Storchaka and Mark Dickinson prefer the following API: python/cpython#112449 (comment)

        Py_hash_t
        hash_double(PyObject *obj, double value)
        {
            if (Py_IS_NAN(value)) {
                return Py_HashPointer(obj);
            }
            else {
                return Py_HashDouble(value);
            }
        }

How should I handle this situation? Enforce C API Working Group decision? Reopen the discussion here? I respect Serhiy and Mark's opinion on IEEE 754 float and hashing since they are expert on this topic.

If API A is clearly said to be "for implementing tp_hash on a float-like type", that the object parameter should be the instance, and that the object parameter is allowed to be NULL (in which case the result when it was necessary is either 0 or an identifiable error code distinct from any other failure reason), I'm okay with that one.

I'm still only barely convinced that this is worth adding a public API, even an unstable one. It's still going to be totally valid to construct an actual float from the value and get its hash, so we have a slow+compatible option already. If the perf is important, copying the function in its entirety and keeping it updated in new versions should be worth the effort. (Though I admit this is mostly about setting a boundary - we do not yet have a problem of making public C API for every single micro-function just for someone who wants both speed and compatibility, but I worry that may become more of a problem.)

If the perf is important, copying the function in its entirety and keeping it updated in new versions should be worth the effort.

This doesn't sound right to me. Perf is definitely important for numpy when doing this for an array (which they do; complex was just the simplest example). The numpy team shouldn't be required to watch the code in that particular file for changes. The API is for sure more stable than the implementation, so they can't get a compiler warning for this -- they'd have to implement a test that dynamically compares the value of the hashes they compute to the hashes computed by the CPython runtime for a wide variety of floats, and that seems just the wrong way about it when we can just export the proper API. Also note that we effectively already export the proper API (albeit as a private function) -- this problem is almost entirely of our own making since we decided we want to make it hard for 3rd parties to use private functions.

How to compute the hash of multicomponent value? You call Py_HashDouble() for every component and combine results. If several components are NaNs, API A will give you the same hash for them. Combining a hash with itself can give more predicable result if you use something simple (like add or xor). You also repeatedly calculate the hash of the pointer.

API B and API C allow to stop when you find a NaN and return Py_HashPointer(obj). With API B you check the result of the call, with API C you check for NaN before the call.

Perf is definitely important for numpy when doing this for an array (which they do; complex was just the simplest example).

For what it's worth, for numpy and this API specifically, we would only be calling this function if someone needs the hash of a float or complex scalar, e.g. if they make one a dict key. Arrays don't have hashes because they are mutable.

Ah, together those are a compelling argument not to care so much about either perf or convenience for numpy. So then I'm okay with all three APIs.

a compelling argument not to care so much about either perf or convenience for numpy

Indeed. If either of those was important, my preferences would be very different :)

For a summary, rather than focus on the history of the proposals, I'd rather focus on the proposals themselves -- specifically, the remaining reasons people have to not prefer the API.
It's hard to judge, as people's understanding and opinions evolve. Maybe it's something like:

API A (double, PyObject *obj) -> Py_hash_t
- assumes a container -- a loss in generality.
API B (double, Py_hash_t *result) -> int
- is slower.
API C (double) -> Py_hash_t
- hides a potentially dangerous edge case.

Anything missing?

If API B's slowness is an issue, I'll be happy to change my preferences (after trying a faster implementation).

@encukou

@encukou

assumes a container -- a loss in generality

I think there may be a misunderstanding here. In the NumPy use-case, there's a scalar value (e.g., of type np.float32) and we want to compute a hash for it in a way that's consistent with equality (e.g., equality with Python floats). That scalar value is a Python object, containing a raw C float. For API A, NumPy would pass that float (cast to double) as the first argument, and the wrapper object as the second argument. I don't think there's any container here (except in the same sense that a Python float can be regarded as a container for a single C double).

Apologies; after rereading, the misunderstanding is mine.

except in the same sense that a Python float can be regarded as a container for a single C double

... and that's exactly the sense of container that you meant. Mea culpa.

IMO the 3 discussed APIs are fine, there are no huge difference between them. Well, some people have opinions about one specific aspect of these APIs: report error, performance, etc.

Now, how can we take a decision to move on?

Now, how can we take a decision to move on?

My first choice is to keep the existing private API and close this issue. My second choice is to rename the existing API to remove the leading underscore without semantic changes (i.e., API A).

Multiple APIs were discussed in the same issue and it's uneasy for me to follow which API is being referred to, so I created a new issue to add a new Py_hash_t Py_HashDouble(double, PyObject *obj) function: #10

Similarly to this, it would be nice to expose a public Py_hash_t Py_HashBytes(const void*, Py_ssize_t) function. This is required to make buffer-like objects hashable in a way that's consistent with Python's bytes and memoryview objects.

Similarly to this, it would be nice to expose a public Py_hash_t Py_HashBytes(const void*, Py_ssize_t) function.

I suggest to open a new issue for this.

@gvanrossum

Apparently, the C API Working Group failed to take a decision on this API.

On January 31st, @gvanrossum wrote on the companion issue #10:

I'm taking an executive decision and closing this issue for lack of progress.

So I also close this issue. numpy will have to continue using the private _Py_HashDouble() function for now.

Navigation Menu

Repository navigation

Add Py_HashDouble() function #2

26 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Add Py_HashDouble() function #2

Description

Activity

zooba commented on Dec 5, 2023

vstinner commented on Dec 5, 2023

zooba commented on Dec 5, 2023

vstinner commented on Dec 5, 2023

vstinner commented on Dec 5, 2023

zooba commented on Dec 7, 2023

vstinner commented on Dec 7, 2023

zooba commented on Dec 7, 2023

vstinner commented on Dec 8, 2023

zooba commented on Dec 8, 2023

vstinner commented on Dec 11, 2023

iritkatriel commented on Dec 11, 2023

gvanrossum commented on Dec 11, 2023

encukou commented on Dec 12, 2023

vstinner commented on Dec 14, 2023

26 remaining items

zooba commented on Jan 10, 2024

gvanrossum commented on Jan 10, 2024

serhiy-storchaka commented on Jan 10, 2024

ngoldbaum commented on Jan 10, 2024

gvanrossum commented on Jan 10, 2024

encukou commented on Jan 11, 2024

mdickinson commented on Jan 11, 2024

mdickinson commented on Jan 11, 2024

vstinner commented on Jan 18, 2024

gvanrossum commented on Jan 18, 2024

vstinner commented on Jan 30, 2024

pitrou commented on Feb 6, 2024

vstinner commented on Feb 6, 2024

vstinner commented on Apr 19, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions