[REVIEW] mdspan-ify rmat_rectangular_gen #833
Conversation
mhoemmen
added
feature request
mhoemmen
force-pushed
the
mdspanify-rmat_rectangular_gen
branch
from
039a3b1 to
1f54a42
Compare
mhoemmen
changed the title
mhoemmen
changed the title
I added two mdspan overloads of rmat_rectangular_gen, each corresponding to one of the existing overloads. I also added tests for the two overloads. Both tests build and pass.
Fix order of parameters (handle, RngState, in, out, params). Remove redundancy in documentation, and make it clear that the mdspan overloads are favored and the raw array overloads are legacy.
mhoemmen
force-pushed
the
mdspanify-rmat_rectangular_gen
branch
from
aa09e62 to
fe9a454
Compare
I added an overload of `raft::random::permute` that takes device mdspan instead of raw arrays, and `std::optional<mdspan<...>>` for optional output arrays. I also added two overloads that let users pass in `std::nullopt`. I've added a unit test that imitates the existing unit test for the raw-array overloads; it builds and passes. The test ensures that the two `std::nullopt` overloads also compile. I also opportunistically fixed some unrelated existing small build errors that were blocking forward progress. That commit is included in PRs #830 and #833 as well, so if it merges, the change should rebase correctly. Authors: - Mark Hoemmen (https://github.com/mhoemmen) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #834
1. Validate sizes of output arrays (if provided) 2. Remove n_edges parameter, since it can be inferred from the output arrays (if provided) 3. Express the idea that the raw pointers interface currently lets users provide either out, (out_src and out_dst), or (out, out_src, and out_dst). Make users pass in the mdspan in a way that prevents run-time errors as early as possible.
mhoemmen
force-pushed
the
mdspanify-rmat_rectangular_gen
branch
from
f63415f to
0ca5c55
Compare
There was a problem hiding this comment.
Once again, thanks for these changes! I keep going back and forth about the added complexity of the additional class vs the oversimplification of just providing more overloads. I really haven't come to a good resolution in my head yet about it so I'm definitely okay deferring your thoughts and opinions there.
If we do go the route of the additional class, though, we should move it out into its own _types.hpp file.
Instead of consolidating overloads by using rmat_rectangular_gen_output, introduce an overload for each of the three cases of output (1, 2, or 3 output vectors).
Address review feedback by moving the helper class rmat_rectangular_generator_output into a new separate header. Remove rmat_rectangular_generator_output's nullopt_t case, as it's no longer possible for users to reach that case.
|
@cjnolet wrote:
I just moved it into its own new header file in The class does add complexity, but it's nice to consolidate all the error code in one place. With a bit more cleverness later, we could reduce code duplication in the class' member functions by using |
|
Sounds great. Let me know when you are ready for this to go in and I'll merge it over |
|
@cjnolet wrote:
It's ready to merge now -- thanks! : - D |
|
@gpucibot merge |
1 similar comment
|
@gpucibot merge |
I added two overloads of
rmat_rectangular_gen, one for each of the existing overloads, that take device mdspan instead of raw arrays. I've added a unit test that imitates the existing unit test for the raw-array overloads; it builds and passes.I also opportunistically fixed some unrelated existing small build errors that were blocking forward progress.