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

Truncated SVD #74

Merged
merged 7 commits into from
Mar 28, 2024
Merged

Truncated SVD #74

merged 7 commits into from
Mar 28, 2024

Conversation

cortner
Copy link
Member

@cortner cortner commented Mar 23, 2024

continue Jerry's PR for adding the truncated SVD

@cortner
Copy link
Member Author

cortner commented Mar 23, 2024

I changed naming to be consistent with Julia conventions. I fix the inline docs, and I messed a bit with one of the test files. It really needs more tests I think but it would be ok to merge this now so it becomes usable.

@wcwitt -- are you happy for us to merge this and tag a new version?

A = randn(Nobs, Nfeat) / sqrt(Nobs)
y = randn(Nobs)
A1 = randn(Nobs, Nfeat) / sqrt(Nobs)
U, S1, V = svd(A)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line breaks the tests because of a typo. I would fix it myself, but I have a question. The changes in this block affect the tests for ALL solvers by redefining A to be something slightly different. I'm not opposed to this in principle, but are you sure it's what you want?

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to see how changing regularisation changes the solution. In the original all singluar values are very close to 1, so for methods like RRQR and truncated SVD varying the regularisation has essentially zero effect.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll fix the typo, sorry I wasn't paying attention there.

@wcwitt
Copy link
Collaborator

wcwitt commented Mar 26, 2024

Thanks - looks interesting!

Once the previous comment is resolved (and the tests are passing) I'm happy for you to merge.

Christoph Ortner added 2 commits March 26, 2024 06:14
@wcwitt
Copy link
Collaborator

wcwitt commented Mar 26, 2024

The CI ran successfully, so happy for you to merge and tag.

@cortner cortner merged commit 2070804 into main Mar 28, 2024
7 checks passed
@cortner
Copy link
Member Author

cortner commented Mar 28, 2024

registered as v"0.1.5"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants