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 view ctors that don't take erased sentinel parameter #165

Merged
merged 5 commits into from
Jun 3, 2022

Conversation

PointKernel
Copy link
Member

@PointKernel PointKernel commented May 26, 2022

This PR adds static_map::device_view/static_map::device_mutable_view constructors that don't take the erased sentinel parameter.

It's a dependency of rapidsai/cudf#10983

@PointKernel PointKernel added type: feature request New feature request topic: static_map Issue related to the static_map Needs Review Awaiting reviews before merging helps: rapids Helps or needed by RAPIDS labels May 26, 2022
@PointKernel PointKernel requested review from jrhemstad May 28, 2022 20:39
Comment on lines +441 to +442
empty_key_sentinel_{empty_key_sentinel.value},
erased_key_sentinel_{empty_key_sentinel.value},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, so we set the erased_key_sentinel to be the same as the empty_key_sentinel. This implies, that we must not call map.erase(...) or we break our probing scheme. In order to ensure this, we have a guard in the host API here.
However, as far as I can see, there is no such guard for the device API.
Is this by design?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't really have a good way to guard it on the device API. The best would could probably do is an assert.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed. We should at least add a disclaimer to the doxygen docs, which are btw missing for the device-sided erase(…) functions here and here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Docs added.

@PointKernel PointKernel merged commit 5a76729 into NVIDIA:dev Jun 3, 2022
@PointKernel PointKernel deleted the add-view-ctors branch June 3, 2022 16:02
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this pull request Jun 7, 2022
Depends on NVIDIA/cuCollections#165 and NVIDIA/cuCollections#171

This is a preparation step to finally address #10841. It fetches the latest version of `cuco` that [adds missing thrust headers ](NVIDIA/cuCollections#161) and [improves `static_map::retrieve_all`](NVIDIA/cuCollections#169).

Authors:
  - Yunsong Wang (https://github.com/PointKernel)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #10983
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 Needs Review Awaiting reviews before merging topic: static_map Issue related to the static_map type: feature request New feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants