kvikio icon indicating copy to clipboard operation
kvikio copied to clipboard

More thoroughly type `*Handle` classes

Open jakirkham opened this issue 1 year ago • 6 comments

  • Type *Handle classes as much as possible
  • Add .pxd files for *Handle objects
  • Convert functions to cpdef where appropriate
  • Convert sizes to Py_ssize_t and use -1 or 0 for default values
  • Simplify and optimize parse_buffer_argument
  • Use asarray on the Python side to ensure we have Arrays in Cython to speedup access

jakirkham avatar Oct 16 '24 20:10 jakirkham

There is arr.pxd in _lib. That said, point taken on the rest of them

At a first pass was thinking about internal usage and simply making that experience better

If we don't ship the .pxd, then it is not actually public for consumers of these packages. CuPy follows this practice currently ( https://github.com/cupy/cupy/issues/130 )

It is possible this could be useful externally. For example could imagine a performant Zarr implementation in Cython might use this

jakirkham avatar Oct 22 '24 02:10 jakirkham

There is arr.pxd in _lib. That said, point taken on the rest of them

True, we have cdef functions that takes Array so we need it in a .pxd.

At a first pass was thinking about internal usage and simply making that experience better If we don't ship the .pxd, then it is not actually public for consumers of these packages. CuPy follows this practice currently ( cupy/cupy#130 )

Right, but I really like to have the external and class declarations close together. If we only use something in one place, it is nice to have it together in a single file IMO.

It is possible this could be useful externally. For example could imagine a performant Zarr implementation in Cython might use this

I suspect that the overhead of calling a Python vs. a Cython function is negligible at this level but it might avoid a GIL grab. I think we should wait until this becomes an issue.

madsbk avatar Oct 22 '24 06:10 madsbk

Ok so is your main recommendation to fold the .pxd files back into .pyx for now?

Are there other things I should consider when doing a second pass?

jakirkham avatar Oct 22 '24 06:10 jakirkham

Ok so is your main recommendation to fold the .pxd files back into .pyx for now?

Yes

Are there other things I should consider when doing a second pass?

I also prefer Optional[int] over a magical number like -1

madsbk avatar Oct 22 '24 07:10 madsbk

Ok can look at that

The Optional[int] bit is a little odd though as we do assign default values (0) for file_offset and dev_offset, which come after. Could we make these consistent somehow? On the Cython side it would be easier if these all had default values that were ints

jakirkham avatar Oct 22 '24 07:10 jakirkham

IMO, having 0 as a default offset argument is good because 0 is a valid argument whereas -1 is not a valid size. Also note, none of the Cython functions has any defaults. It is the pure Python functions that introduces defaults like here so that we don't have to keep multiple defaults in sync.

I get why it is faster with int arguments but I am not sure it is worth the added complexity?

madsbk avatar Oct 22 '24 09:10 madsbk