Skip to content

Conversation

@Cleaf-y
Copy link

@Cleaf-y Cleaf-y commented Mar 10, 2024

Background

This pull request addresses Issue #36 , which reports an exception on Windows within the heapq.pyx module. The error was due to a type mismatch during the initialization of self.cy_heap_index using np.full, expecting 'Py_ssize_t' but receiving 'long'.

Investigation and Solution

The root cause was identified as the lack of an explicit dtype in the np.full function call. To fix this issue, dtype=np.intp was added to the function call. np.intp is the numpy data type designed to be portable across platforms and matches 'ssize_t' in C, which is compatible with 'Py_ssize_t' in Python's C API. This change ensures the correct type is used, aligning with platform-specific requirements, especially on Windows.

Changes Made

Modified the initialization of self.cy_heap_index in heapq.pyx to explicitly set the dtype to np.intp:

From:

self.cy_heap_index = np.full(values.shape, fill_value=-1)

To:

self.cy_heap_index = np.full(values.shape, fill_value=-1, dtype=np.intp)

Conclusion

This fix not only resolves the reported issue but also enhances the codebase's portability and compatibility across different platforms. The explicit specification of dtype=np.intp ensures the heapq.pyx module can be compiled on Windows without encountering the previously reported type mismatch error.

I request a review for this pull request to merge the fix into the main branch, addressing Issue #36 and facilitating broader platform compatibility, Thanks!

…dtype

Addressing Issue malcolmw#36 The expected type was 'Py_ssize_t', but 'long' was provided instead. This led to runtime exception. The solution was to explicitly set the dtype parameter to np.intp in the np.full function call. 

This modification resolves the error on Windows by ensuring the type matches the expected 'Py_ssize_t', as np.intp is the numpy data type that maps to 'ssize_t' on the platform being compiled on, thus aligning with Python's indexing types and C's ssize_t on Windows.
@MHee
Copy link

MHee commented Aug 15, 2024

This worked for me on Windows!

But took me a while to get here.

@malcolmw, please merge the pull-request.

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

Successfully merging this pull request may close these issues.

2 participants