Skip to content
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

Fix loadStateField evaluator to address #1092 #1096

Merged
merged 2 commits into from
Dec 4, 2024
Merged

Conversation

mperego
Copy link
Collaborator

@mperego mperego commented Dec 4, 2024

Apparently linear access of Kokkos dynamic rank views is no longer working.
Not sure if this is a bug (DynRankView is not as widely tested as View) or expected behavior.
Fixes #1092

Apparently linear access of Kokkos dynamic rank views is no longer working
Copy link
Collaborator

@bartgol bartgol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sneaky. That was a great find Mauro, thanks!

Kokkos::RangePolicy<ExecutionSpace>(0,data.size()),
KOKKOS_CLASS_LAMBDA(const int i) {
dataVec[i] = (i < stateToLoad_size) ? stateData(i) : 0.0;
Kokkos::MDRangePolicy<ExecutionSpace, Kokkos::Rank<3>>({0,0,0},{stateData.extent(0),stateData.extent(1),stateData.extent(2)}),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works cause we use DynRankView, so usince 3 indices is always ok (we just have extent(1) and extent(2) that may be 1), right?

Copy link
Collaborator Author

@mperego mperego Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out. That's correct, however this code would fail in debug mode. To avoid the debug check, we have to use access() instead of operator(). Fix coming up soon.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kind of weird that this works for rank<3 but nice that it does since it seems a lot simpler.

Copy link
Collaborator

@mcarlson801 mcarlson801 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! This change looks good to me.

@mperego
Copy link
Collaborator Author

mperego commented Dec 4, 2024

An alternative would be to deep_copy the state view into the MDfiled view, but this is a bit more fragile and it expects the views to have same properties (e.g. Kokkos layouts).

@mcarlson801
Copy link
Collaborator

An alternative would be to deep_copy the state view into the MDfiled view, but this is a bit more fragile and it expects the views to have same properties (e.g. Kokkos layouts).

I can confirm this doesn't work, I tried this awhile back before using an MDField iterator.

Copy link
Collaborator

@jewatkins jewatkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems to point to some issue with MDFieldVectorRight. could you finish removing it from the code? It looks like it's only in a few other places: ScatterScalarResponse, SeparableScatterScalarResponse, and PHAL_Utilities

Kokkos::RangePolicy<ExecutionSpace>(0,data.size()),
KOKKOS_CLASS_LAMBDA(const int i) {
dataVec[i] = (i < stateToLoad_size) ? stateData(i) : 0.0;
Kokkos::MDRangePolicy<ExecutionSpace, Kokkos::Rank<3>>({0,0,0},{stateData.extent(0),stateData.extent(1),stateData.extent(2)}),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kind of weird that this works for rank<3 but nice that it does since it seems a lot simpler.

@mcarlson801
Copy link
Collaborator

this seems to point to some issue with MDFieldVectorRight. could you finish removing it from the code? It looks like it's only in a few other places: ScatterScalarResponse, SeparableScatterScalarResponse, and PHAL_Utilities

The DynRankView that is causing problems is the one in stateData, I'm pretty sure. MDFieldVectorRight is designed to be accessed linearly.

@mperego
Copy link
Collaborator Author

mperego commented Dec 4, 2024

Yes, I agree, stateData was the issue.

Copy link
Collaborator

@jewatkins jewatkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah okay, then it's fine.

@mperego mperego merged commit 238f75f into master Dec 4, 2024
@mperego mperego deleted the mperego/fix1092 branch December 4, 2024 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nightly build failures
4 participants