Skip to content

[FEA] Matrix shift rows and columns #2634

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

Merged
merged 17 commits into from
May 7, 2025

Conversation

jinsolp
Copy link
Contributor

@jinsolp jinsolp commented Apr 15, 2025

Implementation for in-place row or column shifting for matrices.

Supports the following shift-then-fill

  • shifting then filling in with a single value
  • shifting then filling in with a given matrix
  • shifting then filling in with the rowid / colid

Has the following options

  • ShiftDirection: can shift towards the beginning (left for columns, top for rows) or the end (right for columns, bottom for rows).
  • ShiftType: either row or column

Currently needed for the case where we need to add a self loop in a knn graph.

@jinsolp jinsolp requested review from a team as code owners April 15, 2025 22:45
Copy link

copy-pr-bot bot commented Apr 15, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@jinsolp jinsolp added enhancement New feature or request feature request New feature or request non-breaking Non-breaking change and removed enhancement New feature or request labels Apr 15, 2025
@jinsolp jinsolp changed the title [FEA] Matrix column shift [FEA] Matrix shift Apr 25, 2025
@jinsolp jinsolp changed the title [FEA] Matrix shift [FEA] Matrix shift rows and columns Apr 25, 2025
* @param[in] shift_type: ShiftType::ROW shifts rows and ShiftType::COL shift columns
*/
template <typename math_t, typename matrix_idx_t>
void shift_self(raft::resources const& handle,
Copy link
Member

Choose a reason for hiding this comment

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

Can we incorporate this as a specialized input into the prior functions so that we can avoid having a separate function just for this? Every template function we expose becomes another compiled kernel into the user's binary. The more we can consolidate, the better.

Copy link
Contributor Author

@jinsolp jinsolp Apr 29, 2025

Choose a reason for hiding this comment

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

val is now a nullopt. If not specified, we fill with row ids and column ids.

@jinsolp
Copy link
Contributor Author

jinsolp commented Apr 29, 2025

/ok to test

@cjnolet
Copy link
Member

cjnolet commented May 1, 2025

/ok to test be95a4a

@cjnolet
Copy link
Member

cjnolet commented May 7, 2025

/ok to test 7f12f71

Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

LGTM!

@cjnolet
Copy link
Member

cjnolet commented May 7, 2025

/merge

@jinsolp
Copy link
Contributor Author

jinsolp commented May 7, 2025

/ok to test 3c52fba

1 similar comment
@cjnolet
Copy link
Member

cjnolet commented May 7, 2025

/ok to test 3c52fba

@rapids-bot rapids-bot bot merged commit bce5e76 into rapidsai:branch-25.06 May 7, 2025
92 checks passed
@jinsolp jinsolp deleted the matrix-shift branch May 7, 2025 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake cpp feature request New feature or request non-breaking Non-breaking change
Development

Successfully merging this pull request may close these issues.

2 participants