Skip to content

Deprecate MyPair#151

Open
Sichao25 wants to merge 1 commit into
SCOREC:masterfrom
Sichao25:yus/deprecate
Open

Deprecate MyPair#151
Sichao25 wants to merge 1 commit into
SCOREC:masterfrom
Sichao25:yus/deprecate

Conversation

@Sichao25

@Sichao25 Sichao25 commented Aug 9, 2025

Copy link
Copy Markdown
Collaborator

Deprecated the MyPair struct, which is not used in any current functionality. Kokkos::pair is preferred for any scenarios requiring similar data structures.

@cwsmith

cwsmith commented Aug 9, 2025

Copy link
Copy Markdown
Contributor

Thank you @Sichao25. Before this can be merged we need to check that GITRm, XGCm, COMET, and PUMI-Tally aren't using this interface. You should have access to the GITRm and XGCm repos. I'm not sure about PUMI-Tally.

@zhangchonglin Would you please grep for PairView in COMET?

@zhangchonglin

Copy link
Copy Markdown
Contributor

Thank you @cwsmith, @Sichao25. Comet is not using PairView. Actually, I also have to delete part of that function (the use of that) to compile on Aurora:

#ifdef PP_USE_GPU
namespace Kokkos {
  using pumipic::MyPair;
  PP_DEVICE_VAR MyPair ma = MyPair(10000000);
  PP_DEVICE_VAR MyPair mi = MyPair(0);
  template <>
  struct reduction_identity<MyPair> {
    KOKKOS_FORCEINLINE_FUNCTION constexpr static const MyPair& max() {return ma;}
    KOKKOS_FORCEINLINE_FUNCTION constexpr static const  MyPair& min() {return mi;}
  };
}
#endif

@jacobmerson

Copy link
Copy Markdown
Contributor

I was thinking we could start with the ``[[deprecated]]` attribute, but if we can confirm that none of the downstream applications make use of this I'm fine with completely removing it.

@jacobmerson

Copy link
Copy Markdown
Contributor

PumiTally does not use this.
@dhyan1272 do you use any MyPair in GITRM?

@dhyan1272

Copy link
Copy Markdown
Contributor

No I do not use it in GITRm

@cwsmith cwsmith left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Given that we are changing the API (albeit an apparently unused portion), I think we should increase the major version number in CMakeLists.txt. Thoughts?

@jacobmerson

Copy link
Copy Markdown
Contributor

Given that we are changing the API (albeit an apparently unused portion), I think we should increase the major version number in CMakeLists.txt. Thoughts?

I agree.

@jacobmerson

Copy link
Copy Markdown
Contributor

@Sichao25 I think as soon as you bump the version number in the CMakeLists.txt we can merge this.

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.

5 participants