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

Deprecate Kokkos::vector #6252

Merged
merged 6 commits into from
Jul 12, 2023
Merged

Conversation

dalg24
Copy link
Member

@dalg24 dalg24 commented Jul 1, 2023

Rational: Kokkos::vector might have been relevant in the past to ease the transition to Kokkos but this is really not something we want people to use any more.
It is at times the source of confusion (quite different to Kokkos::array, Kokkos::pair, or Kokkos::complex).

I suggest we mark it as deprecated and prepare for removal in Kokkos 5.0

@fnrizzi
Copy link
Contributor

fnrizzi commented Jul 3, 2023

some jenkins are failing bc DKokkos_ENABLE_DEPRECATED_CODE_4 is on, so this PR triggers the expected warning.
the ci failing is a good thing for this PR, isn't it? but seems to me we won't have all test passing for this PR anyway right?

Copy link
Contributor

@fnrizzi fnrizzi left a comment

Choose a reason for hiding this comment

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

caveat the CI comments to be discussed

@masterleinad
Copy link
Contributor

We confirmed in the meeting that we want to deprecate it.

@dalg24
Copy link
Member Author

dalg24 commented Jul 5, 2023

There are a couple unrelated failures (machine issues)

#if defined(KOKKOS_ENABLE_DEPRECATED_CODE_4)
#if defined(KOKKOS_ENABLE_DEPRECATION_WARNINGS)
namespace {
[[deprecated("Deprecated <Kokkos_Vector.hpp> header is included")]] int
Copy link
Member Author

Choose a reason for hiding this comment

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

@masterleinad why did you do this rather than KOKKOS_DEPRECATED_WITH_COMMENT?

Copy link
Contributor

Choose a reason for hiding this comment

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

What I cared about is using "#error" and including "Kokkos_Macros.hpp" but not so much about using KOKKOS_DEPRECATED_WITH_COMMENT as opposed to KOKKOS_ENABLE_DEPRECATION_WARNINGS. The way now at least only declares extra symbols when they are needed for the deprecation warning.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good enough.

To be pedantic, there will be no symbol, that is why I used an anonymous namespace.

@dalg24
Copy link
Member Author

dalg24 commented Jul 6, 2023

Two failures look unrelated. One is lost connection with the runner, this other one is failure to start the container.

@dalg24 dalg24 merged commit 7e6871f into kokkos:develop Jul 12, 2023
27 of 28 checks passed
@dalg24 dalg24 deleted the deprecate_kokkos_vector branch July 12, 2023 18:54
@masterleinad masterleinad mentioned this pull request Jul 14, 2023
dalg24 added a commit to dalg24/kokkos-core-wiki that referenced this pull request May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecate Items to be deprecated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants