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

Mdspanifying (currently tested) raft::matrix #846

Merged

Conversation

cjnolet
Copy link
Member

@cjnolet cjnolet commented Sep 26, 2022

No description provided.

RAFT v22.10 Release automation moved this from PR-WIP to PR-Needs review Sep 29, 2022
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.

EXCELLENT

Copy link
Member

@divyegala divyegala left a comment

Choose a reason for hiding this comment

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

There's quite a few redundant element-wise functions in here. Can you create an issue to convert those to encapsulate any mdspan shape for later on?

/**
* @brief Overload of `sort_keys_per_row` to help the
* compiler find the above overload, in case users pass in
* `std::nullopt` for one or both of the optional arguments.
Copy link
Member

Choose a reason for hiding this comment

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

I thought this overload was only needed when there are two optional arguments? @mhoemmen

* @param[in] scalar scalar value to fill matrix elements
*/
template <typename math_t, typename idx_t, typename layout>
void fill(const raft::handle_t& handle,
Copy link
Member

Choose a reason for hiding this comment

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

We should probably have another element wise function for every possible mdspan shape

RAFT v22.10 Release automation moved this from PR-Needs review to PR-Reviewer approved Oct 2, 2022
@cjnolet
Copy link
Member Author

cjnolet commented Oct 2, 2022

I mentioned this offline but it's still unclear to me what exactly the final state of matrix vs linalg should look like after consolidation. For example, I'm not completely convinced that these element wise ops should be in linalg because they aren't linear algebra, yet because they are element wise ops they might not necessarily belong in the matrix namespace either. We need to think about it a bit more, I think.

@cjnolet
Copy link
Member Author

cjnolet commented Oct 2, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit d265e58 into rapidsai:branch-22.10 Oct 2, 2022
RAFT v22.10 Release automation moved this from PR-Reviewer approved to Done Oct 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake cpp improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants