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

Should we return Symmetric matrices? #63

Closed
nickrobinson251 opened this issue Aug 27, 2019 · 4 comments
Closed

Should we return Symmetric matrices? #63

nickrobinson251 opened this issue Aug 27, 2019 · 4 comments
Labels
enhancement New feature or request

Comments

@nickrobinson251
Copy link
Contributor

No description provided.

@tlienart
Copy link
Collaborator

tlienart commented Aug 27, 2019

That seems reasonable, IIRC in a lot of places the estimators are based upon calls to the default cov (and linear combinations thereof) and so properties should (?) be preserved, though we also compute the uncentered sample covariance which should be wrapped in a Symmetric wrapper and all this should be tested.

@mateuszbaran thoughts? also, I'm potentially happy to PR for this (unless @nickrobinson251 you wanted to take the lead?)

@tlienart tlienart added the enhancement New feature or request label Aug 27, 2019
@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Aug 27, 2019

Cool - go for it! (if @mateuszbaran is onboard ofc) -- i'd be happy to review :)

@mateuszbaran
Copy link
Owner

That's a good idea, although I'd be careful implementing this. Standard cov works for complex random variables but then the matrix is Hermitian. I haven't tested which of our estimators behave correctly for complex numbers (there is a separate issue about it, #29), SimpleCovariance definitely works though and some linear shrinkage estimators might work with minor modifications. The point is, if we do this, we should wrap all matrices in either Symmetric or Hermitian and SimpleCovariance needs to cover both cases.

I'd be happy to review a PR 🙂 .

tlienart added a commit that referenced this issue Sep 3, 2019
makes all returned matrices symmetric
add tests more or less everywhere that things are symmetric
mateuszbaran added a commit that referenced this issue Sep 4, 2019
@mateuszbaran
Copy link
Owner

Fixed by #64.

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

No branches or pull requests

3 participants