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

Add a note about changes in RangePolicy index conversion safety check #558

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ldh4
Copy link
Contributor

@ldh4 ldh4 commented Jul 30, 2024

Changed argument index types from int64_t to IndexType to better describe what's really in the implementation, which is templated type that is required to be convertible to IndexType.
It's already noted that the internal IndexType will be defaulted to int64_t if not specified.

Also added a note about changed behavior in conversion safety check when roundtrip convertibility is not provided (from kokkos/kokkos#7172).

@ldh4 ldh4 changed the title Add a note about the indices conversion checks in RangePolicy Add a note about changes in RangePolicy index conversion safety check Jul 30, 2024
@dalg24
Copy link
Member

dalg24 commented Jul 30, 2024

Changed argument index types from int64_t to IndexType to better describe what's really in the implementation, which is templated type that is required to be convertible to IndexType.

Should we not just it is templated? (not asking you to change, just trying to follow the reasoning)

@ldh4
Copy link
Contributor Author

ldh4 commented Jul 31, 2024

Changed argument index types from int64_t to IndexType to better describe what's really in the implementation, which is templated type that is required to be convertible to IndexType.

Should we not just it is templated? (not asking you to change, just trying to follow the reasoning)

I am for exposing that IndexType is a templated type. It's more accurate that way, but I think if we are to do that, few corrections should also be made throughout this documentation page. More specifically, the fact that index_type and IndexType are somewhat used interchangeably throughout this page should be fixed (including in synopsis).

For instance, constructors should either be

template <typename IndexType1, typename IndexType2,
          std::enable_if_t<(std::is_convertible_v<IndexType1, index_type> &&
          std::is_convertible_v<IndexType2, index_type>),
          bool> = false>
RangePolicy(IndexType1 work_begin, IndexType2 work_end );

or maybe just be:

template <typename IndexType1, typename IndexType2>
RangePolicy(IndexType1 work_begin, IndexType2 work_end );

If either option sounds reasonable, I'll make these changes.

@nliber
Copy link
Contributor

nliber commented Jul 31, 2024

I don't know the right way to do this.

The templated constructors are really just an implementation detail. It is designed to work like non-templated overloads in that we are simulating implicit convertibility to member_type with the additional benefit that we check that no information is being lost. I'd rather we figure out some way to say that. I don't want to say member_type as that is just a bit too abstract, as most people are just using the defaults, passing numbers, and not thinking about it in generic terms.

Documentation should be usage-focused.

@nliber
Copy link
Contributor

nliber commented Aug 1, 2024

Expanding on what I wrote on Slack:

We need a different name than IndexType, as that is also the class template name one passes to change the internal type. We can change it to either index_type or member_type.

Maybe add a note saying it defaults to the size_type of the execution_space, which is typically an unsigned type, and can be changed by passing in IndexType<T> as a template parameter to RangePolicy. I know this is redundant with the description at Execution Policies, but there is a lot to read on there to figure this out.

Even though they are interchangeable, we really should be consistent on which one we use in the docs. I have a slight preference for index_type (the downside being that IndexType and index_type are pronounced the same way, even in my head).

We need to balance simplifying the documentation for users w/o glossing over lower level details they may eventually need.

@ldh4
Copy link
Contributor Author

ldh4 commented Aug 2, 2024

@nliber

Expanding on what I wrote on Slack:

We need a different name than IndexType, as that is also the class template name one passes to change the internal type. We can change it to either index_type or member_type.

I overlooked that there's Kokkos::IndexType. Reverted to index_type.

Maybe add a note saying it defaults to the size_type of the execution_space, which is typically an unsigned type, and can be changed by passing in IndexType<T> as a template parameter to RangePolicy. I know this is redundant with the description at Execution Policies, but there is a lot to read on there to figure this out.

This information is already available on this page in Common Arguments for all Execution Policies section.
Modified IndexType row to make its description consistent with what's described in Execution Policies page.

Even though they are interchangeable, we really should be consistent on which one we use in the docs. I have a slight preference for index_type (the downside being that IndexType and index_type are pronounced the same way, even in my head).

We need to balance simplifying the documentation for users w/o glossing over lower level details they may eventually need.

Although I do also prefer index_type over member_type, index_type is not used anymore in RangePolicy implementation. Only member_type is used.
It doesn't really make any difference since member_type and index_type are the same things, but it makes me wonder if we should prefer member_type everywhere or refactor RangePolicy to use index_type over member_type to avoid confusion.
One problem I have with member_type is that its naming doesn't directly give an idea on what member it is referring to.

@nliber
Copy link
Contributor

nliber commented Aug 2, 2024

If a reader asks "what is member_type or index_type?"

If you look at the synopsis, you see

using member_type = PolicyTraits<Args...>::index_type;
//...
// Inherited from PolicyTraits<Args...>
//...
using index_type = PolicyTraits<Args...>::index_type;

It's a clue, but not particularly helpful. You have to dig through the source to understand what PolicyTraits does (good luck). More likely, if we use index_type, at least people can guess correctly that they may be related. But you still have to guess that it is either IndexType<T> if specified, or "size_type of ExecutionSpaceConcept" if not (which they then have to guess maps to execution_space::size_type). If it is execution_space::size_type, it is probably the unsigned size_t (although in reality it is sometimes it is unsigned, which may or may not match size_t), because that is what it tends to be in the C++ standard library.

That is a lot of mental gymnastics to go through to reason out that "I need numbers and they should probably be unsigned" (and even the "unsigned" part people skip; otherwise, we wouldn't need the round trip test).

Brainstorming here: maybe we just need to say something like:

  • index_type - If IndexType<T> is passed as a template parameter to RangePolicy then T; defaults to execution_space::size_type (which is usually an unsigned number type)
  • member_type - a synonym for index_type

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.

4 participants