Implement unravel_index for row-major array. #631

trivialfis · 2022-04-27T10:15:31Z

Related: #629 Only row-major is currently implemented in this PR. I can work on column-major support if needed. The function is hidden in the detail namespace right now due to the return type (thrust::tuple) can not be directly used in mdspan, which might change in the future. Please see the document and tests for example usage.

The implementation is mostly copied from XGBoost (I'm the author) https://github.com/dmlc/xgboost/blob/fdf533f2b9af9c068cddba50839574c6abb58dc3/include/xgboost/linalg.h#L523 .

trivialfis · 2022-04-27T11:16:11Z

rerun tests

trivialfis · 2022-04-27T12:12:45Z

hmm ? I thought libcu++ is available in raft by default.

cjnolet · 2022-04-27T13:53:04Z

libcu++ is pulled in by cucollections, which is now only enabled when either building tests or the optional "distance" component.

This was done to reduce the dependency burden for projects downstream who aren't using the sparse distances API (which is what depends on cucollections).

trivialfis · 2022-04-27T14:48:19Z

Ah, thank you for sharing. I will try to switch to thrust instead.

trivialfis · 2022-04-28T10:07:35Z

I have changed cuda::std::tuple to thrust tuple. The PR is ready.

divyegala · 2022-04-28T17:27:37Z

cpp/include/raft/detail/mdarray.hpp

+}
+
+/**
+ * \brief Turns linear index into coordinate.  Similar to numpy unravel_index. This is not


I think it should be okay to expose this to public, as we are doing with flatten and reshape in #601 ?

If others think it's public ready then I'm happy to expose it. For me these are the concerns:

what if one day mdspan decides to accept std::extents as an index?

what if we use other implementations of tuple in the future? like cuda::std::tuple or just std::tuple instead of thrust::tuple

@cjnolet had suggested that it's better we rely lesser on thrust as time goes on. I find value in using cuda::std::tuple over thrust::tuple as I have a feeling the latter is going to be replaced by the former. But maybe Corey feels differently about introducing libcu++ back as a core dependency

@trivialfis I prefer to use std::tuple here. I'd like to keep both thrust and libcu++ out of public APIs. C++ stdlib is fine.

@cjnolet but it doesn't always work in CUDA kernel. Also, it's hidden in detail name space.

The challenge w/ header-only here is that the dependencies for all included headers are required at compile-time by consumers downstream. That includes things pulled in transitively from the detail namespace. The goal here is to draw the line and make it so that all the public apis in the core/ directory can be safely exposed by our consumers through their own public APIs while not imposing any additional dependencies on their users outside of RMM and the CTK libs. Currently, our mdarray header in detail pulls in thrust and I'd like to eventually separate that out so that thrust isn't a hard requirement just for including mdarray.

If we want to allow this function to be included by files in core/, I would propose we find or create some other object to contain the unraveled indices. Though, if this is truly internal code and meant to stay that way, maybe we should consider separating this function out into a different header for now which is documented accordingly (for example, mentioning that it uses thrust, so it shouldn't be included by any headers in core/). Maybe something like detail/mdarray_internal_utils.hpp just to be very explicit?

I will hide this function as an internal function. Closing this PR for now and will submit it again when it's actually used.

divyegala · 2022-04-28T17:29:47Z

cpp/include/raft/detail/mdarray.hpp

+template <typename LayoutPolicy, std::size_t... Exts>
+MDSPAN_INLINE_FUNCTION auto unravel_index(size_t idx,
+                                          detail::stdex::extents<Exts...> shape,
+                                          LayoutPolicy const&)


Why is this function arg needed? To stay compatible with NumPy args? I think it's acceptable that we are able to get this information directly from the template type, and thus to remove this arg

Depends on how you like to call the function:

unravel_index<stdex::layout_rigth>(idx, shape)

or

unravel_index(idx, shape, stdex::layout_right{})

I chose the second one as it feels more aligned with mdspan design:

submdspan(array, std::full_extent_t{}) // use constructor of full_extent_t here

Hmmm, that is fair. I see value in deduced template types as compared to explicit

divyegala · 2022-04-28T17:31:27Z

cpp/include/raft/detail/mdarray.hpp

+template <typename Fn,
+          typename Tup,
+          std::size_t kTupSize = thrust::tuple_size<std::remove_reference_t<Tup>>::value>
+MDSPAN_INLINE_FUNCTION auto constexpr apply(Fn&& f, Tup&& t) -> decltype(auto)


The trailing return type here is redundant

it's required to denote that the return type might be a reference.

auto&& for universal references?

Actually, I'm not sure if this will work. I'm okay with the former

divyegala · 2022-04-28T17:34:49Z

cpp/include/raft/detail/mdarray.hpp

+}
+
+/**
+ * C++ 17 style apply for thrust tuple.


Very much like this, and that it just works. I wager this would be very useful in accessing and assigning for COOs

vyasr · 2022-05-09T16:41:22Z

rerun tests

cjnolet · 2022-05-16T14:01:56Z

cpp/include/raft/detail/mdarray.hpp

+}
+
+/**
+ * \brief Turns linear index into coordinate.  Similar to numpy unravel_index. This is not


@trivialfis I prefer to use std::tuple here. I'd like to keep both thrust and libcu++ out of public APIs. C++ stdlib is fine.

trivialfis · 2022-05-16T14:26:18Z

@cjnolet but it doesn't always work in CUDA kernel. Also, it's hidden in detail name space.

Revised version of #631 with `thrust::tuple` replaced by `std::tuple` and custom `apply` function replaced by `std::apply`. Related: #629 The implementation is mostly copied from XGBoost https://github.com/dmlc/xgboost/blob/fdf533f2b9af9c068cddba50839574c6abb58dc3/include/xgboost/linalg.h#L523 . In the tests, I have used both `__host__ __device__` and `__device__` lambdas to make sure `std::tuple` and `std::apply` are working correctly with CUDA kernel. Authors: - Jiaming Yuan (https://github.com/trivialfis) Approvers: - Divye Gala (https://github.com/divyegala) - Corey J. Nolet (https://github.com/cjnolet) URL: #723

trivialfis added 3 commits April 27, 2022 18:04

Implement unravel index for row-major array.

4af551c

format.

9427773

Avoid change.

b79b13b

trivialfis requested a review from a team as a code owner April 27, 2022 10:15

trivialfis added the 3 - Ready for Review label Apr 27, 2022

github-actions bot added the cpp label Apr 27, 2022

trivialfis added feature request New feature or request non-breaking Non-breaking change and removed cpp labels Apr 27, 2022

github-actions bot added the cpp label Apr 27, 2022

cleanup.

0f6eaa3

Use thrust tuple instead.

fd6d0a7

cjnolet assigned trivialfis Apr 27, 2022

divyegala reviewed Apr 28, 2022

View reviewed changes

cjnolet requested changes May 16, 2022

View reviewed changes

trivialfis closed this May 16, 2022

trivialfis mentioned this pull request Jun 24, 2022

[FEA] Implement unravel_index for row-major array. #723

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement unravel_index for row-major array. #631

Implement unravel_index for row-major array. #631

trivialfis commented Apr 27, 2022 •

edited

Loading

trivialfis commented Apr 27, 2022

trivialfis commented Apr 27, 2022

cjnolet commented Apr 27, 2022

trivialfis commented Apr 27, 2022

trivialfis commented Apr 28, 2022 •

edited

Loading

divyegala Apr 28, 2022

trivialfis Apr 28, 2022

divyegala Apr 28, 2022

cjnolet May 16, 2022

cjnolet May 16, 2022

trivialfis May 16, 2022

divyegala Apr 28, 2022

trivialfis Apr 28, 2022 •

edited

Loading

divyegala Apr 28, 2022

divyegala Apr 28, 2022

trivialfis Apr 28, 2022

divyegala Apr 28, 2022

divyegala Apr 28, 2022

divyegala Apr 28, 2022

vyasr commented May 9, 2022

cjnolet May 16, 2022

trivialfis commented May 16, 2022

Implement unravel_index for row-major array. #631

Implement unravel_index for row-major array. #631

Conversation

trivialfis commented Apr 27, 2022 • edited Loading

trivialfis commented Apr 27, 2022

trivialfis commented Apr 27, 2022

cjnolet commented Apr 27, 2022

trivialfis commented Apr 27, 2022

trivialfis commented Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trivialfis Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyasr commented May 9, 2022

Choose a reason for hiding this comment

trivialfis commented May 16, 2022

trivialfis commented Apr 27, 2022 •

edited

Loading

trivialfis commented Apr 28, 2022 •

edited

Loading

trivialfis Apr 28, 2022 •

edited

Loading