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

Optimization for moving least squares #1000

Merged
merged 10 commits into from
Dec 29, 2023

Conversation

mrlag31
Copy link
Contributor

@mrlag31 mrlag31 commented Dec 27, 2023

This PR modifies the SVD pseudo-inverse and the coefficients computation, its main goal was to reduce the amount of global memory used, use scratch memory and compound the kernels.

@aprokop aprokop added the performance Something is slower than it should be label Dec 27, 2023
@aprokop aprokop mentioned this pull request Dec 27, 2023
@aprokop
Copy link
Contributor

aprokop commented Dec 27, 2023

@mrlag31 Can you please explain in more details what the difference is?

@mrlag31
Copy link
Contributor Author

mrlag31 commented Dec 27, 2023

  • The previous SVD implementation used two extra square matrices (one for the diagonalisation, one for the unit matrices). Now, it uses a vector and a square matrix, the diagonalisation is done on the input matrix.
  • The coefficients computation is now batched per-thread using a team policy. All of the previous kernels were compounded into a single one and all of the temporary matrices are now placed in shared memory.

Copy link
Contributor

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

Drive-by comments

Copy link
Contributor

@aprokop aprokop left a comment

Choose a reason for hiding this comment

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

I'm not sure about renaming in symmetricPseudoInverseSVDKernel. I don't think the new names make it any clearer, and would prefer the old names.

@mrlag31
Copy link
Contributor Author

mrlag31 commented Dec 28, 2023

I'm not sure about renaming in symmetricPseudoInverseSVDKernel. I don't think the new names make it any clearer, and would prefer the old names.

I felt like the old ones were harder to understand. The new ones directly explains, by their name, what they are in regards to the formula, whereas the old ones were a bit too abstract.

Copy link
Contributor

@aprokop aprokop left a comment

Choose a reason for hiding this comment

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

I'm ok with the current state.

@aprokop aprokop merged commit 30172a6 into arborx:master Dec 29, 2023
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Something is slower than it should be
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants