Kokkos reverse mode improvements

[gojakuch]

No description provided.

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.37%. Comparing base (183719a) to head (56e4faa).
Report is 2 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1116   +/-   ##
=======================================
  Coverage   94.37%   94.37%           
=======================================
  Files          50       50           
  Lines        8366     8366           
=======================================
  Hits         7895     7895           
  Misses        471      471           

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[vgvassilev]

@kliegeois, @brian-kelley can you take a look?

[brian-kelley]

This looks fine to me! My only suggestion is that you could probably get away with hardcoding size_t as the type for all the resize dimensions and their derivatives, since those arguments to Kokkos::resize are always size_t.

[vgvassilev]

This looks fine to me! My only suggestion is that you could probably get away with hardcoding size_t as the type for all the resize dimensions and their derivatives, since those arguments to Kokkos::resize are always size_t.

Thanks for the comment. I am confused, are you suggesting reduce the template specializations to one which takes a size_t argument?

[brian-kelley]

@vgvassilev size_t wouldn't have to be a template argument anymore, but the View being resized still does. So instead of

template <typename View, typename Idx0, typename Idx1, typename Idx2,
          typename Idx3, typename Idx4, typename Idx5, typename Idx6,
          typename Idx7, typename dIdx0, typename dIdx1, typename dIdx2,
          typename dIdx3, typename dIdx4, typename dIdx5, typename dIdx6,
          typename dIdx7> ...

it could just be

template <typename View> ...

[gojakuch]

This looks fine to me! My only suggestion is that you could probably get away with hardcoding size_t as the type for all the resize dimensions and their derivatives, since those arguments to Kokkos::resize are always size_t.

I think it's better to keep these as templates, since sometimes Clad generates numerical values of different types depending on the argument and thus may fail to find this custom derivative in some edge cases (such as when using both literal and variable indices of different types as arguments).

[vgvassilev]

This looks fine to me! My only suggestion is that you could probably get away with hardcoding size_t as the type for all the resize dimensions and their derivatives, since those arguments to Kokkos::resize are always size_t.

I think it's better to keep these as templates, since sometimes Clad generates numerical values of different types depending on the argument and thus may fail to find this custom derivative in some edge cases (such as when using both literal and variable indices of different types as arguments).

Can we check if that is really limitation? If we can drop all of these template arguments it would be become significantly more readable.

[gojakuch]

I've rebased this PR and managed to reduce the number of lines by having std::size_t arguments in some cases. seems like it's no longer an issue there.

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[vgvassilev]

@brian-kelley, were these changes what you were aiming for?

[brian-kelley]

@vgvassilev @gojakuch Yes, that's what I had in mind. Thanks!