Add tests for raft::matrix
#937
Conversation
cjnolet
added
improvement
| @@ -170,7 +169,7 @@ void printHost(const m_t* in, idx_t n_rows, idx_t n_cols) | |||
| */ | |||
| template <typename m_t, typename idx_t = int> | |||
| __global__ void slice( | |||
| m_t* src_d, idx_t m, idx_t n, m_t* dst_d, idx_t x1, idx_t y1, idx_t x2, idx_t y2) | |||
| const m_t* src_d, idx_t m, idx_t n, m_t* dst_d, idx_t x1, idx_t y1, idx_t x2, idx_t y2) | |||
There was a problem hiding this comment.
I'm a little concerned that changing these is going to ultimately break the existing APIs. We could cast away the constness for now (and create a github issue for it) or test this PR in cuml to make sure it doesn't break anythign.
| void binary_mult_skip_zero(const raft::handle_t& handle, | ||
| raft::device_matrix_view<math_t, idx_t, layout_t> data, | ||
| raft::device_vector_view<const math_t, idx_t> vec, | ||
| Apply apply) |
There was a problem hiding this comment.
I like the idea of making this an enum but I suggest we find a more descriptive name for this. Apply just brings to mind several different possibilities, none of which immediately said "oh broadcast the vector across the rows" to me.
Maybe something like BroadcastType? (That's just off the top of my head, im sure there are better names)
There was a problem hiding this comment.
Or maybe even VectorBroadcast::ALONG_ROWS?
There was a problem hiding this comment.
I just realized this was already defined in linalg (prior to this PR). I think we can keep this name for now and rename this in a future PR.
| template <typename math_t, typename extents, typename layout> | ||
| void fill(const raft::handle_t& handle, | ||
| raft::device_mdspan<math_t, extents, layout> inout, | ||
| math_t scalar) |
There was a problem hiding this comment.
This should be a host scalar view as well to match the other overload.
There was a problem hiding this comment.
This was intended to be a much easier version to use because scalar is not used as a "out parameter". I don't see the need for it to be host scalar view.
Otherwise we'd have to create a host scalar view that holds a pointer to zero every time we want to reset to zero a matrix?
And the in parameter seems to be useless in the other overload as well when looking at the implementation details.
There was a problem hiding this comment.
I'm actually thinking we can probably hold off on adding the host scalar view version altogether for now until we have a need to support the device scalar view. These are more generally useful when we have a pointer to device memory that we want to use as input but we don't want to synchronize the stream each time.
|
@gpucibot merge |
Linking #877.
Implementation of the remaining tests for
raft::matrix