Skip to content

Make device contains take a template key parameter#174

Merged
PointKernel merged 16 commits intoNVIDIA:devfrom
PointKernel:template-contains
Jun 13, 2022
Merged

Make device contains take a template key parameter#174
PointKernel merged 16 commits intoNVIDIA:devfrom
PointKernel:template-contains

Conversation

@PointKernel
Copy link
Member

@PointKernel PointKernel commented Jun 4, 2022

Contributes to #26

Closes #172

This PR makes the device contains APIs take a template key parameter. It also includes

  • update CI script to unblock CI failure
  • clean up tests by using thrust::identity

@PointKernel PointKernel added type: feature request New feature request topic: static_map Issue related to the static_map helps: rapids Helps or needed by RAPIDS topic: static_multimap Issue related to the static_multimap labels Jun 4, 2022
@PointKernel
Copy link
Member Author

rerun tests

@PointKernel PointKernel requested a review from jrhemstad June 7, 2022 19:20
Key const& k,
Hash hash = Hash{},
KeyEqual key_equal = KeyEqual{}) const noexcept;
__device__ std::enable_if_t<std::is_invocable_v<KeyEqual, ProbeKey, Key>, bool> contains(
Copy link

Choose a reason for hiding this comment

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

Suggested change
__device__ std::enable_if_t<std::is_invocable_v<KeyEqual, ProbeKey, Key>, bool> contains(
__device__ std::enable_if_t<std::is_invocable_r_v<bool, KeyEqual, ProbeKey, Key> && std::is_invocable_r_v<bool, KeyEqual, Key, ProbeKey>, bool> contains(

Copy link
Member Author

Choose a reason for hiding this comment

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

The condition here is not a requirement check. It's just to differentiate CG API and non-CG API cause if we pass three template parameters to contains, the compiler cannot know which one we are referring to. e.g. https://github.com/PointKernel/cuCollections/blob/3b0adf597ed828f3813030452fb39f9b1735c90b/tests/static_map/custom_type_test.cu#L217

@ttnghia
Copy link

ttnghia commented Jun 10, 2022

This looks good to me. I will try to test and will update.

@ttnghia
Copy link

ttnghia commented Jun 10, 2022

Update: I have successfully adopted this: rapidsai/cudf#10656 (code).

Thanks again for working on it.

ProbeKey const& k,
KeyEqual key_equal) noexcept
{
static_assert(std::is_invocable_r_v<bool, KeyEqual, ProbeKey, Key>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

tbh, all these static_asserts are overkill. If someone passes in an invalid callable, it will still fail to compile, just with a different error message. This feels like a ton of complexity to maintain.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I pointed to is_invocable just for the purposes of documentation. I didn't intend that we need to static_assert all of the requirements of the function.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed those static_assert as requested and updated corresponding docs.

I agree that so many asserts are painful to maintain now but the situation will be improved after the grand refactor where we only need one set of those static asserts in the base class as opposed to putting them in every involved function for all map types.

@PointKernel PointKernel requested a review from jrhemstad June 13, 2022 16:32
@PointKernel PointKernel added the topic: CI CI issue label Jun 13, 2022
@PointKernel PointKernel merged commit 0a832bc into NVIDIA:dev Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

helps: rapids Helps or needed by RAPIDS topic: CI CI issue topic: static_map Issue related to the static_map topic: static_multimap Issue related to the static_multimap type: feature request New feature request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEA] Support contains with template key types

3 participants