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.

Sign up for GitHub

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

Jump to bottom

Add Block Krylov SVD method in CF #3413

Merged
merged 13 commits into from Feb 17, 2023
Merged

Add Block Krylov SVD method in CF #3413

merged 13 commits into from Feb 17, 2023

Conversation

AdarshSantoria
Copy link
Contributor

No description provided.

@AdarshSantoria AdarshSantoria mentioned this pull request Feb 12, 2023
Merged
rcurtin
rcurtin reviewed Feb 13, 2023
View reviewed changes
src/mlpack/methods/cf/decomposition_policies/block_krylov_svd_method.hpp Outdated

// Do singular value decomposition using the block krylov SVD algorithm.
RandomizedBlockKrylovSVD blockkylovsvd;
blockkylovsvd.Apply(data, w, sigma, h, rank);
Copy link
Member

Choose a reason for hiding this comment

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

Here too we have the same issue as the QUIC_SVD PR with what to do with sigma. 👍

AdarshSantoria and others added 2 commits February 13, 2023 22:02
@AdarshSantoria @rcurtin
Update src/mlpack/methods/cf/decomposition_policies/block_krylov_svd_…
2eff5e0 
…method.hpp

Co-authored-by: Ryan Curtin <ryan@ratml.org>
@AdarshSantoria
fix typo
7e9e8de
@AdarshSantoria AdarshSantoria mentioned this pull request Feb 13, 2023
Merged
rcurtin
rcurtin reviewed Feb 13, 2023
View reviewed changes
Copy link
Member

@rcurtin rcurtin left a comment

Choose a reason for hiding this comment

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

I think that everything here is correct---if you can handle the small comments and the build passes, then I think we are good to merge it. 👍

src/mlpack/methods/cf/decomposition_policies/block_krylov_svd_method.hpp Outdated Show resolved Hide resolved
src/mlpack/methods/cf/decomposition_policies/block_krylov_svd_method.hpp Outdated Show resolved Hide resolved
AdarshSantoria and others added 2 commits February 14, 2023 21:41
@AdarshSantoria @rcurtin
Update src/mlpack/methods/cf/decomposition_policies/block_krylov_svd_…
47e0b03 
…method.hpp

Co-authored-by: Ryan Curtin <ryan@ratml.org>
@AdarshSantoria @rcurtin
Update src/mlpack/methods/cf/decomposition_policies/block_krylov_svd_…
41bc55b 
…method.hpp

Co-authored-by: Ryan Curtin <ryan@ratml.org>
rcurtin
rcurtin approved these changes Feb 15, 2023
View reviewed changes
Copy link
Member

@rcurtin rcurtin left a comment

Choose a reason for hiding this comment

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

Thanks---this looks good to me too. Once #3404 is merged, there will be a merge conflict here, but it should not be hard to work out. 👍

mlpack-bot[bot]
mlpack-bot bot approved these changes Feb 16, 2023
View reviewed changes
Copy link

@mlpack-bot mlpack-bot bot left a comment

Choose a reason for hiding this comment

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

Second approval provided automatically after 24 hours. 👍

@rcurtin
Copy link
Member

rcurtin commented Feb 17, 2023

Nice, thank you for adding it to cf_main.cpp too! 👍

@rcurtin rcurtin merged commit 7f60e29 into mlpack:master Feb 17, 2023
@AdarshSantoria AdarshSantoria deleted the bksvd branch February 22, 2023 12:58
@rcurtin rcurtin mentioned this pull request Apr 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rcurtin rcurtin rcurtin approved these changes

@mlpack-bot mlpack-bot[bot] mlpack-bot approved these changes

Assignees
No one assigned
Labels
s: unanswered s: unlabeled
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@AdarshSantoria @rcurtin