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.

Sign up for GitHub

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

Jump to bottom

#254: Clean up reconstruction logic for View, DynRankView, and DynamicView #300

Merged
merged 8 commits into from
Mar 31, 2023
Merged

#254: Clean up reconstruction logic for View, DynRankView, and DynamicView #300

merged 8 commits into from
Mar 31, 2023

Conversation

PhilMiller
Copy link
Member

Fixes #254

@PhilMiller
Copy link
Member Author

This depends on at least Kokkos 3.7. I don't want to charge ahead with merging yet without at least a little bit of thought about the test that I had to disable because of kokkos/kokkos#5609

It may be worth just going ahead, and we can run the concrete sibling test on real CUDA systems. It was mostly there as a TDD thing.

@PhilMiller PhilMiller requested review from lifflander, cz4rs and nmm0 March 30, 2023 19:04
@PhilMiller PhilMiller marked this pull request as ready for review March 30, 2023 22:06
@PhilMiller
Copy link
Member Author

On further consideration, I think we can leave that test disabled for now, and maybe turn it back on if/when Kokkos loosens up or otherwise improves usability of logical memory spaces.

cz4rs
cz4rs approved these changes Mar 31, 2023
View reviewed changes
Copy link
Contributor

@cz4rs cz4rs left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@cz4rs
Copy link
Contributor

cz4rs commented Mar 31, 2023

On further consideration, I think we can leave that test disabled for now, and maybe turn it back on if/when Kokkos loosens up or otherwise improves usability of logical memory spaces.

Should we create a follow-up issue or maybe include a reference to kokkos/kokkos#5609 in the disabled test?

@PhilMiller PhilMiller mentioned this pull request Mar 31, 2023
Open
@PhilMiller
Copy link
Member Author

Follow-up issue created. Merging.

@PhilMiller PhilMiller merged commit ab1174f into develop Mar 31, 2023
@PhilMiller PhilMiller deleted the 254-simplify-view branch March 31, 2023 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cz4rs cz4rs cz4rs approved these changes

@lifflander lifflander Awaiting requested review from lifflander

@nmm0 nmm0 Awaiting requested review from nmm0

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Simplify View/DynRankView reconstruction after Kokkos 3.7 ships
2 participants
@PhilMiller @cz4rs