Conversation
|
FYI, #1978 will likely require some changes in your kernel function signatures to match the changes there |
ab79a98 to
e5e08d3
Compare
33ac013 to
5010a34
Compare
|
now that #1978 is merged, I know I need to adapt the kernels. I'll do that. for now, committing what I've got so far and rebasing |
pratikvn
left a comment
There was a problem hiding this comment.
Nice work! For the most part, I think the sturucture looks good! I think some kernels can be merged.
| exec->run(rs::make_init_cf(cf_marker)); | ||
| exec->run(rs::make_rs_coarsening(rs_op, is_strong.get_const_data(), | ||
| lambda.get_data(), cf_marker)); | ||
| exec->run(rs::make_rs_cleanup(cf_marker)); |
There was a problem hiding this comment.
A general rule: If you are calling kernels consecutively, they can probably be merged into a single kernel.
As much as possible, you want to have as few kernel launches as possible.
| exec->run(rs::make_fill_coarse_rows(cf_marker, coarse_rows.get_data())); | ||
|
|
||
| array<IndexType> fine_to_coarse(exec, fine_dim); | ||
| exec->run( | ||
| rs::make_fill_fine_to_coarse(cf_marker, fine_to_coarse.get_data())); |
There was a problem hiding this comment.
Same here. fine_dim is already known before-hand, so you can allocate the array before, and combine these kernels.
| rs_op = rs_op_shared_ptr.get(); | ||
| this->set_fine_op(rs_op_shared_ptr); | ||
| } | ||
|
|
There was a problem hiding this comment.
I think it might be reasonable to add a validation check here that the matrix is an M-matrix, and if not throw. At a later point, we can hide it under a flag, or move it to the data_validation compoenent of the matrix.
| // * A = | ||
| // * [ 2 -1 0 ] | ||
| // * [ -1 2 -1 ] | ||
| // * [ 0 -1 2 ] |
There was a problem hiding this comment.
I think this is a bit trivial. Can you choose a slightly larger matrix ? Maybe like 5x5 ?
This PR provides the reference implementation for RS-coarsening