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

refactor div-conq SVD, impl svd_rand #165

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

nlhepler
Copy link
Contributor

credit to @pmarks for original svd_rand impl

This should probably be 2 PRs, but I wanted to post it here for discussion first.

First, I moved svddc to svd_dc (so we can have svd_rand), and moved svd_dc under the SVD_ trait, de-duplicating a lot of logic.

I also implement svd_rand, though this version does not have the changes required for complex numbers, which we definitely want. This is dependent on a set of complex trait requirements that just works for Array2, but is needed for sparse representations of matrices (be they sprs or a low-rank representation like M = u.dot(&v).

I also add an optional feature on sprs, though right now only @pmarks branch of sprs implements the Dot traits required to make svd_rand function.

credit to @pmarks for original svd_rand impl
Cargo.toml Outdated
@@ -41,6 +41,10 @@ default-features = false
version = "0.3"
default-features = false

[dependencies.sprs]
git = "https://github.com/pmarks/sprs.git"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is required for the Dot impl in @pmarks branch, which itself is dependent on rust-lang/rust#55437

Copy link
Member

Choose a reason for hiding this comment

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

We need to create nightly CI

Copy link
Member

@termoshtt termoshtt left a comment

Choose a reason for hiding this comment

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

Great!
As you said, it will be better to split into dense and sparse part, and svddc refactoring part possibly.
This PR can be merged by omitting sparse part.

Before implementing sparse part, we need to create nightly CI for testing it, and CI setting will be my task.

Cargo.toml Outdated
@@ -41,6 +41,10 @@ default-features = false
version = "0.3"
default-features = false

[dependencies.sprs]
git = "https://github.com/pmarks/sprs.git"
Copy link
Member

Choose a reason for hiding this comment

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

We need to create nightly CI

FlagSVD::None
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

cool :)

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.

2 participants