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

RangePolicy constructor updates from Release 4.3 #523

Merged
merged 8 commits into from
May 24, 2024

Conversation

nliber
Copy link
Contributor

@nliber nliber commented May 9, 2024

This updates the existing RangePolicy constructor documentation as per kokkos/kokkos#6845 and adds a section for the CTAD constructors as per kokkos/kokkos#6850.

This addresses issue #519 and part of issue #515.

nliber added 3 commits May 9, 2024 15:08
Get rid of extraneous inline, const, etc.
Fix parameters passed to ctors
Fix return type of set_chunk_size()
docs/source/API/core/policies/RangePolicy.rst Outdated Show resolved Hide resolved
Comment on lines 74 to 86
// until 4.3
template<class ... Args>
inline RangePolicy( const execution_space & work_space
, const member_type work_begin
, const member_type work_end
, Args ... args);
RangePolicy( const execution_space & work_space
, member_type work_begin
, member_type work_end
, Args ... args );

// until 4.3
template<class ... Args>
inline RangePolicy( const member_type work_begin
, const member_type work_end
, Args ... args);
RangePolicy( member_type work_begin
, member_type work_end
, Args ... args );

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we even want to bother mentioning the previous interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was a little torn on this. I'll remove it. The only reason to keep it is to match compilation errors with the implementation (even though we don't guarantee any particular implementation).

Kokkos::RangePolicy<ARGS>(Space(), begin, end, args...)
Kokkos::RangePolicy<...>(begin, end)
Kokkos::RangePolicy<...>(begin, end, chunk_size)
Kokkos::RangePolicy<...>(Space(), begin, end)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Kokkos::RangePolicy<...>(Space(), begin, end)
Kokkos::RangePolicy(exec, begin, end)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to have both template parameters and CTAD constructors, as I don't want the usage to imply this is not a templated class.

inline RangePolicy();
RangePolicy();

RangePolicy( member_type work_begin
Copy link
Member

Choose a reason for hiding this comment

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

I'd say index_type
I think using member type was an attempt to have genericity between exec policy but it clearly does not work with MDRangePolicy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to index_type.


Preconditions:
^^^^^^^^^^^^^^

* The start index must not be greater than the end index.
Copy link
Member

Choose a reason for hiding this comment

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

You referred to the PR that check that the indices arguments are representable as index_type.
Maybe that would be confusing because we pretend that the constructors take member_type/index_type by value when in reality they are templated.
Can we really pretend it is QOI?
(this does not need to be resolved here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a sentence under "Preconditions" (I might move it, though).

docs/source/API/core/policies/RangePolicy.rst Outdated Show resolved Hide resolved
Comment on lines 50 to 51
RangePolicy(const RangePolicy&) = default;
RangePolicy(RangePolicy&&) = default;
Copy link
Member

Choose a reason for hiding this comment

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

I would have gotten rid of these guys but I suppose we can do that later

@dalg24 dalg24 merged commit 7ddf508 into kokkos:main May 24, 2024
1 check passed
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.

3 participants