mdspan view for IVF-PQ API #1236
Conversation
cjnolet
added
improvement
There was a problem hiding this comment.
Just a few minor things. overall the PR looks great!
We're also going to need to update the python API to remove all the pointer-based runtime APIs and invoke the mdspan-based APIs directly (you can see python/pylibraft/pylibraft/neighbors/refine.pyx for an example of how to do this).
viclafargue
force-pushed
the
ivf-pq-mdspan
branch
from
cbb2d2e to
126677c
Compare
|
|
||
|
|
||
| cdef device_matrix_view[uint64_t, uint64_t, row_major] \ | ||
| get_device_matrix_view_uint64(array, check_shape=True) except *: |
There was a problem hiding this comment.
This is really great! I'm glad that we're centralizing these. I might suggest using the nomenclature: make_device_matrix_view_xxx though just to be consistent w/ the C++ layer (since we really want to mirror the C++ layer here as much as we can while still being somewhat "pythonic" in the APIs). I'm okay doing that as a follow-on though
There was a problem hiding this comment.
Sure, I am working on IVF reconstruction at the moment, but I can do this as a follow-up PR.
|
@viclafargue i think this needs a style update. One thing that can make this easier is setting up pre-commit hooks (if you haven't already) to make sure the style is correct by the time the commit is made. You can find the instructions here in the contributors guide: https://docs.rapids.ai/api/raft/nightly/contributing.html#python-pre-commit-hooks |
|
/merge |
|
/merge |
|
@cjnolet I am keeping an eye on this PR! It looks like |
|
Thanks @divyegala! I admit that I forgot I had already added the /merge command. Looks like most of the PRs are failing with conda issues at the moment. |
Answers #1209