Extra SparsePack indexers

[adamdempsey90]

PR Summary

Working with Metadata::None fields in artemis that are defined as e.g.,

pkg->AddField<name>(Metadata({Metadata::None, Metadata::OneCopy},                    \
                                 std::vector<int>({nx1})));

To index into this in a pack, parthenon currently supports

pack(b, name)(idx);
pack(b, name, 0, 0, idx);

This MR adds the ability to index as,

pack(b, name, i);
pack(b, name, j, i);

which is a little cleaner.

Tested in artemis. I don't think this clashes with the other available indexers (at least in artemis), but we may not cover everything.

PR Checklist

• Code passes cpplint
• New features are documented.
• Adds a test for any bugs fixed. Adds tests for new features.
• Code is formatted
• Changes are summarized in CHANGELOG.md
• Change is breaking (API, behavior, ...)
  • Change is additionally added to CHANGELOG.md in the breaking section
  • PR is marked as breaking
  • Short summary API changes at the top of the PR (plus optionally with an automated update/fix script)
• CI has been triggered on Darwin for performance regression tests.
• Docs build
• (@lanl.gov employees) Update copyright on changed files

[Yurlungur]

LGTM! Would it be possible to unify the index access methods with something like this? Or would that be ambiguous with the other definitions?

  template <class TIn, typename... Args, REQUIRES(sizeof...(Args) > 0), REQUIRES(IncludesType<TIn, Ts...>::value)>
  KOKKOS_INLINE_FUNCTION auto &operator()(const int b, const TIn &t, const Args... args) const {
    PARTHENON_DEBUG_REQUIRE(!flat_, "Accessor cannot be used for flat packs");
    const int vidx = GetLowerBound(b, t) + t.idx;
    return pack_(0, b, vidx)(std::forward<Args>(args)...);
  }

Also ping @lroberts36

[adamdempsey90]

LGTM! Would it be possible to unify the index access methods with something like this? Or would that be ambiguous with the other definitions?

  template <class TIn, typename... Args, REQUIRES(sizeof...(Args) > 0), REQUIRES(IncludesType<TIn, Ts...>::value)>
  KOKKOS_INLINE_FUNCTION auto &operator()(const int b, const TIn &t, const Args... args) const {
    PARTHENON_DEBUG_REQUIRE(!flat_, "Accessor cannot be used for flat packs");
    const int vidx = GetLowerBound(b, t) + t.idx;
    return pack_(0, b, vidx)(std::forward<Args>(args)...);
  }

Also ping @lroberts36

Yeah, that seems better. I've made the change with a slight modification to Args&&... args.

[lroberts36]

LGTM! Would it be possible to unify the index access methods with something like this? Or would that be ambiguous with the other definitions?

  template <class TIn, typename... Args, REQUIRES(sizeof...(Args) > 0), REQUIRES(IncludesType<TIn, Ts...>::value)>
  KOKKOS_INLINE_FUNCTION auto &operator()(const int b, const TIn &t, const Args... args) const {
    PARTHENON_DEBUG_REQUIRE(!flat_, "Accessor cannot be used for flat packs");
    const int vidx = GetLowerBound(b, t) + t.idx;
    return pack_(0, b, vidx)(std::forward<Args>(args)...);
  }

Also ping @lroberts36

Yeah, that seems better. I've made the change with a slight modification to Args&&... args.

That looks reasonable, I might also add a REQUIRES(all_implement<integral(Args...)>::value) just to be safe.

[lroberts36]

Approved, but needs a changelog update

[adamdempsey90]

Approved, but needs a changelog update

Done

[adamdempsey90]

LGTM! Would it be possible to unify the index access methods with something like this? Or would that be ambiguous with the other definitions?

  template <class TIn, typename... Args, REQUIRES(sizeof...(Args) > 0), REQUIRES(IncludesType<TIn, Ts...>::value)>
  KOKKOS_INLINE_FUNCTION auto &operator()(const int b, const TIn &t, const Args... args) const {
    PARTHENON_DEBUG_REQUIRE(!flat_, "Accessor cannot be used for flat packs");
    const int vidx = GetLowerBound(b, t) + t.idx;
    return pack_(0, b, vidx)(std::forward<Args>(args)...);
  }

Also ping @lroberts36

Yeah, that seems better. I've made the change with a slight modification to Args&&... args.

That looks reasonable, I might also add a REQUIRES(all_implement<integral(Args...)>::value) just to be safe.

Done

[pgrete]

Should this get a quick unit test in the existing sparse pack machinery?