-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds row scatter functionality to dense #1356
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the function description and one test variable
Kudos, SonarCloud Quality Gate passed! |
core/matrix/dense.cpp
Outdated
dim<2> expected_dim{row_idxs->get_num_elems(), this->get_size()[1]}; | ||
GKO_ASSERT_EQUAL_DIMENSIONS(expected_dim, this); | ||
GKO_ASSERT_EQUAL_COLS(this, target); | ||
// @todo check that indices are inbounds for target |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure whether to check the indices here because it will affect the performance. I guess it will have the same runtime as make_row_scatter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would also be my expectation. Perhaps it could be handled by setting an error code in the kernel and then check that afterward. I will need to think if that can be realized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an in-kernel check to make sure we don't write out-of-bounds. I'm open to discussions on how to handle that, because I don't think there are many places where we already use something like this. In principle, we could also do this for the gather kernels, although I don't think the issue is as big there, since we are only reading out-of-bounds. Also, we could wrap the check in some #ifdef
to disable it perhaps in release builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a full review yet, just some quick comments/questions?
include/ginkgo/core/matrix/dense.hpp
Outdated
/** | ||
* Copies this matrix into the given rows of the target matrix. | ||
* | ||
* @tparam IndexType the index type, either int32 or int 64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @tparam IndexType the index type, either int32 or int 64 | |
* @tparam IndexType the index type, either int32 or int64 |
@@ -539,6 +543,42 @@ class Dense | |||
ptr_param<const LinOp> beta, | |||
ptr_param<LinOp> row_collection) const; | |||
|
|||
/** | |||
* Copies this matrix into the given rows of the target matrix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to be a bit more expressive in what it means to "copy this matrix into the given rows". Could you add an example here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, since this is the brief description, I think it's fine. But the detailed description is mangled up in the description of the parameters, so perhaps moving that into a separate description block is better.
include/ginkgo/core/matrix/dense.hpp
Outdated
/** | ||
* Copies this matrix into the given rows of the target matrix. | ||
* | ||
* @tparam IndexType the index type, either int32 or int 64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @tparam IndexType the index type, either int32 or int 64 | |
* @tparam IndexType the index type, either int32 or int64 |
ptr_param<LinOp> target) const; | ||
|
||
/** | ||
* Copies this matrix into the given rows of the target matrix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above.
l<T>({{3.0, 2.7, 6.5}, | ||
{0.7, 1.1, 4.0}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused, looks to me like row 0 and 1 should be switched? Doesn't permute_idxs{exec, {1, 0, 4}}
say {3.0, 2.7, 6.5}
goes into row 1 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An index set is always ordered. So permute_idxs{exec, {1, 0, 4}}
is equivalent to permute_idxs{exec, {0, 1, 4}}
. I will add a comment to row_scatter
bb553d5
to
7d52f8c
Compare
d6223ed
to
39ae497
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs 82.5% Coverage The version of Java (11.0.3) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17. |
currently version with index_set as input is not implemented for devices Co-authored-by: Yu-Hsiang M. Tsai <[email protected]>
caab3d0
to
ecc1b8c
Compare
Co-authored-by: Yu-Hsiang M. Tsai <[email protected]>
ecc1b8c
to
3247327
Compare
Quality Gate passedThe SonarCloud Quality Gate passed, but some issues were introduced. 18 New issues |
This PR add row scatter functionality to dense. The target rows can be specified either by a plain
array<index_type>
or byindex_set<index_type>
. Note that the index_set implies that the target rows are ordered. Signature:Todo: