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 function to convert mdspan to a const view #1188

Merged
merged 10 commits into from
Feb 1, 2023

Conversation

lowener
Copy link
Contributor

@lowener lowener commented Jan 26, 2023

make_const_mdspan is a helper function to convert mdspan<T> into mdspan<const T>.
I added examples of it's usage
@mhoemmen @Nyrio

@lowener lowener added 3 - Ready for Review feature request New feature or request non-breaking Non-breaking change labels Jan 26, 2023
@lowener lowener requested a review from a team as a code owner January 26, 2023 11:42
@github-actions github-actions bot added the cpp label Jan 26, 2023
@lowener lowener self-assigned this Jan 26, 2023
Copy link
Contributor

@Nyrio Nyrio left a comment

Choose a reason for hiding this comment

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

Thanks for this!

@mhoemmen
Copy link
Contributor

mhoemmen commented Jan 26, 2023

Thanks for writing this!

This facility serves an analogous purpose as std::view::as_const.

https://en.cppreference.com/w/cpp/ranges/as_const_view

The use case example here

https://en.cppreference.com/w/cpp/ranges/constant_range

looks very much like the use cases you would like to support. The idea is that you would like to use mdspan<const T, ...> to express that an mdspan parameter is a view-of-const, but you would like the function nevertheless to be callable by users with mdspan<T, ...>.

cpp/bench/matrix/argmin.cu Show resolved Hide resolved
cpp/include/raft/core/mdspan.hpp Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Base: 87.99% // Head: 87.99% // No change to project coverage 👍

Coverage data is based on head (3e3facb) compared to base (92820d5).
Patch has no changes to coverable lines.

Additional details and impacted files
@@              Coverage Diff              @@
##           branch-23.02    #1188   +/-   ##
=============================================
  Coverage         87.99%   87.99%           
=============================================
  Files                21       21           
  Lines               483      483           
=============================================
  Hits                425      425           
  Misses               58       58           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@mhoemmen mhoemmen left a comment

Choose a reason for hiding this comment

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

Thanks for considering this design change! I find that this makes the code clean and easy to extend.

Optionally, if you want users to be able to add support for their custom accessors, you might want to consider a customization point object or tag_invoke design for accessor_of_const.

cpp/include/raft/core/mdspan.hpp Outdated Show resolved Hide resolved
host_device_accessor<std::experimental::default_accessor<std::add_const_t<ElementType>>, MemType>
accessor_of_const(host_device_accessor<std::experimental::default_accessor<ElementType>, MemType> a)
{
return {a};
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the correct way to do it, as long as host_device_accessor has an element type converting constructor like default_accessor does (see the first constructor here). A straightforward unit test would fail to compile without that constructor.

Copy link
Member

Choose a reason for hiding this comment

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

This is a great point, @mhoemmen. It's nice to see the new function being used in the k-means tests but we should probably have a dedicated testcase for this (ideally in the mdspan cpp test file).

@cjnolet
Copy link
Member

cjnolet commented Feb 1, 2023

/merge

@rapids-bot rapids-bot bot merged commit 6a7e125 into rapidsai:branch-23.02 Feb 1, 2023
@lowener lowener deleted the 23.02-const-mdspan branch February 1, 2023 17:42
ahendriksen pushed a commit to ahendriksen/raft that referenced this pull request Feb 2, 2023
`make_const_mdspan` is a helper function to convert `mdspan<T>` into `mdspan<const T>`.
I added examples of it's usage
@mhoemmen @Nyrio

Authors:
  - Micka (https://github.com/lowener)
  - Corey J. Nolet (https://github.com/cjnolet)

Approvers:
  - Louis Sugy (https://github.com/Nyrio)
  - Corey J. Nolet (https://github.com/cjnolet)
  - Mark Hoemmen (https://github.com/mhoemmen)

URL: rapidsai#1188
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review cpp feature request New feature or request non-breaking Non-breaking change
Projects
Development

Successfully merging this pull request may close these issues.

5 participants