Matrix eigendecomposition by loufranco · Pull Request #95 · Jounce/Surge

loufranco · 2019-02-08T23:59:45Z

This PR tries to bring the almost-done eigendecomposition branch into Surge

I expect there to be some comments (it is not lint warning free right now)

I got the older branch updated and fixed conflicts
I changed the interface to support complex vector and matrix returns
I added tests based on Intel LAPACK and numpy docs

But,

My return is a 3-tuple, which generates a lint warning
I am using (Double, Double) for complex numbers. I don't see a convention in Surge for this, but if there is one, let me know
I can't use Matrix with that, so I use [[(Double, Double)]]

…ctors for a given Matrix. Further changes may be appropriate

…master

Refactoring and reformatting

- dgeev wants matrices in column major form - dgeev returns 3 things, so add all to return - decomposition results in complex results, which we represent as a 2-tuple

alejandro-isaza

Thanks! Just a few style comments.

alejandro-isaza · 2019-02-09T00:03:00Z

+// https://software.intel.com/sites/products/documentation/doclib/mkl_sa/11/mkl_lapack_examples/dgeev.htm
+private func buildEigenVector(wi: [Double], v: [Double], result: inout [[(Double, Double)]]) {
+    let n = wi.count
+    for r in 0..<n {


I'm not a fan of one-letter variable names, let use better names.

alejandro-isaza · 2019-02-09T00:04:11Z

+// .0: The eigenvalues represented as an array of complex numbers
+// .1: The left eigenvectors represented as a 2-dimensional array of complex numbers
+// .2: The right eigenvectors represented as a 2-dimensional array of complex numbers
+public func eigendecompostion(_ x: Matrix<Float>) -> ([(Float, Float)], [[(Float, Float)]], [[(Float, Float)]]) {


Use three slashes for doc comments /// also use swift doc comment style. See https://nshipster.com/swift-documentation/

Let's define a struct for the return type.

alejandro-isaza · 2019-02-09T00:05:01Z

+// .0: The eigenvalues represented as an array of complex numbers
+// .1: The left eigenvectors represented as a 2-dimensional array of complex numbers
+// .2: The right eigenvectors represented as a 2-dimensional array of complex numbers
+public func eigendecompostion(_ x: Matrix<Double>) -> ([(Double, Double)], [[(Double, Double)]], [[(Double, Double)]]) {


Same comments as above.

alejandro-isaza · 2019-02-09T00:05:21Z

+    // dgeev_ needs column-major matrices
+    var mat: [__CLPK_doublereal] = transpose(x).grid
+
+    var N1 = __CLPK_integer(x.rows)


variable names should be lowercase, also let's use better names.

loufranco · 2019-02-09T00:34:56Z

The build failure is because of strict lint issues with the 3-tuples I used

I'd like some guidance on the lint warning about using a tuple of 3 items. I could

Suppress it
Make a struct to hold the return
Only return the right eigenvector (this is what numpy does), and use a 2-tuple -- this would make the function have a very analogous signature to numpy. I don't know how important having the left eigenvector is.

- better variable names - use a struct for the decomposition result - use standard Swift doc comments

loufranco · 2019-02-09T11:09:06Z

@alejandro-isaza I implemented your suggestions.

alejandro-isaza

Just a couple more things.

alejandro-isaza · 2019-02-09T14:03:56Z

+///   - x: a square matrix
+/// - Returns: a struct with the eigen values and left and right eigen vectors using (Double, Double)
+///   to represent a complex number.
+public func eigendecompostion(_ x: Matrix<Double>) -> MatrixEigenDecompositionResult<Double> {


This is a lot better. Let's make this function a verb. What about decompose? or eigenDecompose? or decomposeSpectrally? Also missing triple slash in the first three lines.

alejandro-isaza · 2019-02-09T14:05:24Z

+    let decompositionJobV = UnsafeMutablePointer<Int8>(mutating: ("V" as NSString).utf8String)
+    // Call dgeev to find out how much workspace to allocate
+    dgeev_(decompositionJobV, decompositionJobV, &matrixRowCount, &matrixGrid, &eigenValueCount, &eigenValueRealParts, &eigenValueImaginaryParts, &leftEigenVectorWork, &leftEigenVectorCount, &rightEigenVectorWork, &rightEigenVectorCount, &workspaceQuery, &workspaceSize, &error)
+    assert(error == 0, "Matrix not eigen decomposable")


Asserts are bad here because they will be ignored in release build which will probably make the below code crash. Let's make a enum EigenDecompositionError and throw that.

I will fix -- I was trying to follow the conventions in the rest of the file (and Surge seems generally not to throw), but I agree that it's better.

- rename func to eigenDecompose - fix some doc comments - throw instead of asserting - added a test for non-square matrices

loufranco · 2019-02-09T14:54:40Z

@alejandro-isaza ok -- throw is in (with a test for non-square). As far as I can tell, there is no real matrix that isn't decomposable to complex results, but since DGEEV could return an error, I throw -- there is no test I could write to test it, though.

alejandro-isaza · 2019-02-09T15:18:19Z

Thanks again!

Ben Robbins and others added 8 commits April 20, 2015 23:16

Added Two function that calculate the eigen-values and right eigen ve…

34035e7

…ctors for a given Matrix. Further changes may be appropriate

Simple formatting changes

bbee785

Merge branch 'master' of https://github.com/Shlufi/Surge into Shlufi-…

a273c5d

…master

[Issue Jounce#14] Getting PR to compile

331f7bc

Refactoring and reformatting

Fix error in specification of workspace size in call to dgeev_

2e844ca

Merge branch 'master' into matrix-eigendecomposition

0c31581

Added a simple test with only real results

114d86c

Fixed bugs in eigendecomposition and added tests

90f8822

- dgeev wants matrices in column major form - dgeev returns 3 things, so add all to return - decomposition results in complex results, which we represent as a 2-tuple

loufranco mentioned this pull request Feb 9, 2019

Added Functions for EigenValues and EigenVectors #14

Closed

alejandro-isaza reviewed Feb 9, 2019

View reviewed changes

Implement PR Feedback

3ecae4d

- better variable names - use a struct for the decomposition result - use standard Swift doc comments

alejandro-isaza reviewed Feb 9, 2019

View reviewed changes

Respond to PR Feedback

1c826ab

- rename func to eigenDecompose - fix some doc comments - throw instead of asserting - added a test for non-square matrices

alejandro-isaza approved these changes Feb 9, 2019

View reviewed changes

alejandro-isaza merged commit a6dd5f6 into Jounce:master Feb 9, 2019

This was referenced Feb 9, 2019

EigenValue decomposition #49

Closed

why isn't eigen solver in master branch? #55

Closed

regexident mentioned this pull request Oct 1, 2019

Fix member access-levels of MatrixEigenDecompositionResult<Scalar> #132

Merged

Uh oh!

Conversation

loufranco commented Feb 8, 2019

Uh oh!

alejandro-isaza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loufranco commented Feb 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loufranco commented Feb 9, 2019

Uh oh!

alejandro-isaza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loufranco commented Feb 9, 2019

Uh oh!

alejandro-isaza commented Feb 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

loufranco commented Feb 9, 2019 •

edited

Loading