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

Improve macros documentation #27

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

Conversation

cz4rs
Copy link
Contributor

@cz4rs cz4rs commented May 21, 2022

May 16, 2022 Documentation Bootcamp

@cz4rs cz4rs force-pushed the bootcamp-macros branch from 9344f92 to 17d39cf Compare May 21, 2022 20:01
API-Reference/Macros.md Outdated Show resolved Hide resolved
API-Reference/Macros.md Outdated Show resolved Hide resolved
API-Reference/Macros.md Outdated Show resolved Hide resolved
API-Reference/Macros.md Outdated Show resolved Hide resolved
@@ -338,6 +338,7 @@ The following options control `find_package` paths for CMake-based TPLs:

#### Architecture Keywords
Architecture-specific optimizations can be enabled by specifying `-DKokkos_ARCH_X`.
See the [complete list of architecture keywords](../API-Reference/Macros.md#architectures).

* Kokkos_ARCH_A64FX
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we consider removing the list below (as duplicated) and only leave the link to API-Reference/Macros.md?

Copy link
Contributor

@ldh4 ldh4 May 25, 2022

Choose a reason for hiding this comment

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

The only concern I have about removing the list below is that there is a very subtle capitalization difference between the keywords listed on this page and the ones in API-Reference/Macros.md.
Here, it is Kokkos_ARCH_X whereas in Macros.md, it's KOKKOS_ARCH_X.
Though for CMake builds, using either doesn't seem to make any difference...

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's debatable whether users should be able to rely on the existence of these macros.

The CMake option always looks like Kokkos_ENABLE... (although we also define local CMake variables KOKKOS_ENABLE...) while the macro is KOKKOS_ENABLE....

Copy link
Member

Choose a reason for hiding this comment

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

note I don't think that KOKKOS_ENABLE is valid from our side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO:

Copy link
Contributor

@ajpowelsnl ajpowelsnl Jun 27, 2022

Choose a reason for hiding this comment

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

@crtrott , @masterleinad -- Just to make sure I understand the principle here, from the few comments above, I gather that you are proposing:

  1. Users should not use Kokkos configuration macros, that these configuration macros are/ should be mostly / exclusively for developers (i.e., things "under the hood" in Kokkos);
  2. the CMake variables, Kokkos_ENABLE_X , Kokkos_ARCH_XYZ should be the only user-facing configuration variables ?
  3. CMake variables will take precedence over the configuration macros.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider removing the list below (as duplicated) and only leave the link to API-Reference/Macros.md?

@cz4rs and @ldh4 -- Perhaps we could consider:

  1. Making a clear delineation of what should vs. what should not be user facing (e.g., do not make configuration macros user facing, but Kokkos CMake variables should absolutely be public);
  2. Removing duplicated, out-of-date / obsolete tables;
  3. Provide clear explanation of what a Kokkos CMake variable is (i.e., Kokkos_ENABLE_ABC, Kokkos_ENABLE_ARCH_XYZ ) and its intended use, and clearly also state that configuration macros are not part of the public API.

@fnrizzi
Copy link
Collaborator

fnrizzi commented Jun 24, 2022

@cz4rs please update this so that you use the new sphinx branch.
All editable sources are under source/ (the wiki-md-files-to-port directory will be deleted soon).

@fnrizzi
Copy link
Collaborator

fnrizzi commented Jul 1, 2022

is this ready? if not, can you please make it draft?

Sure. How do I do that?

@cz4rs cz4rs force-pushed the bootcamp-macros branch from 5dc0878 to 46cb16c Compare July 1, 2022 17:15
@cz4rs cz4rs marked this pull request as draft July 1, 2022 18:04
@cz4rs cz4rs force-pushed the bootcamp-macros branch 6 times, most recently from 0909be1 to ccb1efc Compare July 11, 2022 16:53
@cz4rs cz4rs marked this pull request as ready for review January 4, 2023 21:53
@cz4rs cz4rs requested a review from fnrizzi January 9, 2023 09:02
@cz4rs cz4rs marked this pull request as draft March 2, 2023 20:17
@cz4rs
Copy link
Contributor Author

cz4rs commented Mar 2, 2023

Converting to draft to fix conflicts and check recent changes.

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.

6 participants