-
Notifications
You must be signed in to change notification settings - Fork 37
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
Thread UIDs through data_collection
#1060
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.
In general, looks good to me and nice refactor along the way.
I just don't understand the unit test. Shouldn't 222 != 22
as in hv2(0) != hxv2(0)
?
Maybe I'm misreading sth.
src/interface/mesh_data.cpp
Outdated
template <typename T> | ||
std::shared_ptr<MeshBlock> MeshData<T>::GetBlock(const int i, const MeshData<T> *src, | ||
const Mesh *pmesh) const { |
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.
Is the GetBlock
functionality potentially universal, i.e., applicable beyond the Inititalize
call?
I'm asking as GetBlock
is a pretty universal name, so if it's special to the Initialize
call, I suggest to name it more specific (so that the general GetBlock
call remains available for future use).
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.
@lroberts36 should jump in here---it's not quite universal, I think... as the MeshData<T> *src
piece specifies this as being for Initialize
. I will change it to GetSourceBlock_
.
tst/unit/test_data_collection.cpp
Outdated
// reset vars | ||
par_for( | ||
loop_pattern_flatrange_tag, "init vars", DevExecSpace(), 0, 0, | ||
KOKKOS_LAMBDA(const int i) { v2(0) = 222; }); |
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.
What is this doing?
Shouldn't this match the xv2(0)=22
call below?
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.
No it shouldn't---this inits v2 to the wrong value as a check that it is overwritten when it is called again on line 108 in the new mesh data object, which is not shallow.
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 didn't actually write this logic, I copied it from the lines above for the add call by string. I'm fine to remove it if you like.
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 a comment to this effect.
Thanks for the review @pgrete I think I've addressed your comments now. |
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 not sure this works with general partitioning, but maybe I am misunderstanding things. See my comments below.
@lroberts36 thanks for the review. I think I've addressed all your comments. |
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.
LGTM
With the latest changes the distinction between |
@pgrete: Yeah, that is no longer required since the |
PR Summary
This PR adds the ability to request a new
MeshData
orMeshBlockData
object by requesting a subset of unique IDs of variables from the parent object, rather than by using only variable names. This should make UID-based operations more usable and thus unlock a bit of performance by minimizing string comparisons. Needed in riot and I suspect also KHARMA.PR Checklist