Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Py_hash_t Py_HashDouble(double, PyObject *obj) function #10

Closed
1 of 5 tasks
vstinner opened this issue Jan 30, 2024 · 5 comments
Closed
1 of 5 tasks

Add Py_hash_t Py_HashDouble(double, PyObject *obj) function #10

vstinner opened this issue Jan 30, 2024 · 5 comments

Comments

@vstinner
Copy link

API: Py_hash_t PyHash_Double(double value, PyObject *obj)

  • If value is a finite number, is inf or -inf, return the hash of value. Similar to PyObject_Hash(PyFloat_FromDouble(value)).
  • If value is not-a-number (NaN) and obj is not NULL, return Py_HashPointer(obj) -- other Python implementation can use a different implementation for this case.
  • Otherwise, if value is not-a-number (NaN) and obj is NULL, return PyHASH_NAN (0).

Pull request: python/cpython#112095

Differences with the existing private API Py_hash_t _Py_HashDouble(PyObject *inst, double v):

  • Exchange parameters order: in the proposed public API, the most important argument, the double parameter, comes first.
  • PyObject* pointer can be NULL in the proposed public API.

Accepting NULL is a trade off with alternative APIs which have no PyObject* argument, proposed previously:

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

The PyObject* parameter avoids asking the caller to implement their own code for the not-a-number case using Py_HashPointer(), PyObject_GenericHash(), or something else. Python 3.10 changed hash(float) to return hash(id(obj)) if the float is not-a-number. Writing such code working on all Python implementation can be tricky. Proposed Py_hash_t PyHash_Double(double value, PyObject *obj) API hides the implementation details, so using this proposed API is more portable.

Main user of the existing private _Py_HashDouble() function: numpy, link to code.

#if PY_VERSION_HEX > 0x030a00a6
#define Npy_HashDouble _Py_HashDouble
#else
static inline Py_hash_t
Npy_HashDouble(PyObject *NPY_UNUSED(identity), double val)
{
    return _Py_HashDouble(val);
}
#endif

static npy_hash_t
@lname@_arrtype_hash(PyObject *obj)
{
    return Npy_HashDouble(obj, (double)PyArrayScalar_VAL(obj, @name@));
}

More background on the Py_HashDouble() API in the previous issue: #2

Alternative: give up trying to add a public C API and comes back to Python 3.12 status quo with the private API Py_hash_t _Py_HashDouble(PyObject *inst, double v).

Votes:

@gvanrossum
Copy link

As I said before in the other issue, I have soured on "fixing" this. I'd rather keep the status quo. If we have to do something (but I don't think we do), the proposed API looks like it's just removing the leading _, so that would be my second choice. But I'm not ticking the box yet.

@encukou
Copy link
Collaborator

encukou commented Jan 31, 2024

IMO, this API is too tightly tied to one specific use in NumPy (and to CPython's internal use). It trades one implementation detail (NaN handling) for another (using a PyObject* container/wrapper).

I'd be OK with exposing it as PyUnstable_, but i don't think it's a good fit for the general C API.
I'd be OK with the status quo (_Py_HashDouble, with an underscore), but I'd treat that more as relieving pressure on us to fix low-priority issues, rather than a long-term fix.

@gvanrossum
Copy link

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

@gvanrossum gvanrossum closed this as not planned Won't fix, can't repro, duplicate, stale Jan 31, 2024
@encukou
Copy link
Collaborator

encukou commented Feb 1, 2024

Does that mean we should revert to 3.12 (_Py_HashDouble), or take no action (remove the function without replacement)?

@gvanrossum
Copy link

Revert to 3.12.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants