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

Implement SVD by divide-and-conquer #164

Merged
merged 1 commit into from
Jun 27, 2019
Merged

Conversation

nlhepler
Copy link
Contributor

Obviously I've made a lot of arbitrary choices here (such as the name. SVDDC? SDD? erk).


use super::{SVDOutput, into_result};

pub trait SVDDC_: Scalar {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could for instance merge this with the existing SVD_ trait, and change the trait to provide another function (svddc, or sdd). This may be preferred.

Or alternatively a more complicated way to invoke the existing svd that switches things around depending.

Copy link
Member

Choose a reason for hiding this comment

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

I'm planning to refactoring these basis of Lapack traits (SVD_ and others) into single trait, but it should be another PR. Your implementation is enough match to current impls, and I will accept it.

@nlhepler
Copy link
Contributor Author

Thinking forward to rand_svd, perhaps a configuration like the following is suggested?

#[repr(u8)]
pub enum SVDFlag {
    Full = b'A',
    Some = b'S',
    None = b'N',
}

pub enum SVDConfig {
    SVD { u: SVDFlag, vt: SVDFlag },  // update gesvd wrappers to support 'S'
    SDD { flag: SVDFlag },
    Rand { k: usize, l: usize, n_iter: usize },
}

impl SVDConfig {
    pub fn svd(u: SVDFlag, vt: SVDFlag) -> Self {
        SVDConfig::SVD { u, v }
    }

    pub fn sdd(flag: SVDFlag) -> Self {
        SVDConfig::SDD { flag }
    }

    pub fn rand(k: usize, l: Option<usize>, n_iter: Option<usize>) -> Self {
        let l = l.unwrap_or(k + 2);
        let n_iter = n_iter.unwrap_or(2);
        SVDConfig::Rand { k, l, n_iter }
    }
}

@termoshtt
Copy link
Member

Thinking forward to rand_svd, perhaps a configuration like the following is suggested?

this seems good. I prefer svd_rand instead of rand because a.svd_rand(...) is better identifiable than a.rand()

@termoshtt
Copy link
Member

The unification of SVD flags as you suggested in comment should be done after introducing random SVD, and another PR. I merge this PR as it is. Thanks a lot!

@termoshtt termoshtt merged commit 7685117 into rust-ndarray:master Jun 27, 2019
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

2 participants