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

Mark as_view_of_rank_n as KOKKOS_FUNCTION #5248

Merged
merged 2 commits into from
Jul 21, 2022

Conversation

masterleinad
Copy link
Contributor

We are using this function in other KOKKOS_FUNCTION functions.

Copy link
Contributor

@PhilMiller PhilMiller left a comment

Choose a reason for hiding this comment

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

Thanks. Sorry I missed this when I added it

Copy link
Member

@crtrott crtrott left a comment

Choose a reason for hiding this comment

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

Approving, come to think of that however it feels like the specialization of as_view_of_rank_n for normal Views probably should move to Kokkos_DynRankView too.

dalg24
dalg24 previously approved these changes Jul 21, 2022
@PhilMiller
Copy link
Contributor

Approving, come to think of that however it feels like the specialization of as_view_of_rank_n for normal Views probably should move to Kokkos_DynRankView too.

I think I had some reason for keeping it there, perhaps to support other things that might need genericity over rank (hello, Sacado), separate from DynRankView. I don't recall exactly, though. Generally, I think it's probably good form to not have the implementation for some bit of generic code on View living in an optional container

if (v.rank() != N) {
Kokkos::Impl::throw_runtime_exception(
Copy link
Contributor

Choose a reason for hiding this comment

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

Switching from an exception to Kokkos::abort means that the unit test for the error case can no longer catch it.

Copy link
Member

Choose a reason for hiding this comment

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

Death test is an option but doing it on the device gets annoying pretty quickly considering the current support in various backends

Copy link
Contributor

Choose a reason for hiding this comment

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

Having seen the error case test working previously, I'd be ok with dropping it

@dalg24 dalg24 dismissed their stale review July 21, 2022 11:29

Tests do not pass

@dalg24
Copy link
Member

dalg24 commented Jul 21, 2022

Failure are unrelated (machine ran out of disk space)

@dalg24 dalg24 merged commit 967f1a0 into kokkos:develop Jul 21, 2022
@PhilMiller PhilMiller mentioned this pull request Jul 26, 2022
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.

4 participants