-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
eigs function b̶r̶o̶k̶e̶n̶ has a confusing interface #3014
Comments
Hm, that looks wrong indeed. Thanks for reporting. Anyone able to dive into this bug with |
I can provide some code, but I don't know how general it is. It would need to be ported to math.js syntax. @josdejong do you want me to make PR or post it somewhere here so someone could take it on? |
Thanks! Both will help, though I guess a PR will be closest to a concrete fix. |
Created a draft, so someone can get inspired :) |
@been-jamming is it [-2, 1] for sure? wolfram alpha shows different results (I just want to be sure) |
@bartekleon You're right, |
Thanks for the PRs (#3016, #3017) and your feedbacks @bartekleon and @been-jamming. I indeed have the feeling too that there is some more generic issue in play, and that we should figure that out rather than implementing a special case for a 2x2 matrix. I did try out how
|
I will try making more generalized version this week. If possible, test cases would be nice. (I don't know how big / many / edge cases I should make so contributions are welcome) |
Thanks Bartosz! |
Sorry I didn't have a chance to look at this issue before now. I do not think there is any serious difficulty with the current behavior of eigs in mathjs, just some misunderstandings: (1) eigs(A) by design returns the matrix E whose columns are eigenvectors. That's the canonical way they are presented, because then AE equals the matrix ΛE, where Λ = diag(λ) is the diagonal matrix of eigenvalues. However if in mathjs you let So the quick upshot is that to get the eigenvector corresponding to the first eigenvalue, you want Now I do agree this is nevertheless somewhat confusing, even though I hope I've justified that it's really the only reasonable way for the function to work. So a couple of things we might do: (a) rename the property of eigs(A) from (2) Then it was hard to recognize the transposition that was going on because of the "funny" numbers in the returned E. After all, we know that "the" eigenvectors of A are (1,2) and (-1, 1). But the fact is there is no such thing as "the" eigenvectors: if v is an eigenvector, then so is kv for any scalar k. Hence (1/3, 2/3) and (1,-1) are just as good a pair of eigenvectors, as are 2.42535(1,2) and -0.97014(-1,1) -- which just happens to be the pair of eigenvectors that this particular algorithm finds. I do not think it's worth significant effort to replace mathjs's current eigs function, given its maturity (been through a lot of overhauls above!) unless someone in this conversation has a proposal that will be demonstrably faster/more accurate/better in some other metric. But I think a PR for either of (a) or (b) would be a good idea -- renaming the issue to reflect that. |
Thanks for investigating the issue, Glen! However, my 2¢ are that changing the eigs API should at least be considered, as it confused even the project owner and the guy who implemented a part of it 😅 EDIT: Oh, now I recall even thinking about how to improve the API, and failing to. The problem is that in Math.js a two-dimensional array of numbers is automatically interpreted as a row matrix. So it is literally impossible to return an array of vectors and not have users confuse it with a row matrix. If there ever was Math.js 2, I would strongly argue against that decision, having now seen too many confusing edge-cases this leads to. But right now, it is baked deep inside Math.js, so there's no coming back. EDIT 2: How about renaming the prop to |
Right, I was trying to say that in my analysis, but may not have been clear enough. That's the central problem here. I do think some changes are in order, and a PR will be needed for #2879 anyway, so there is a good opportunity here. Note that because of defective matrices, the eigenvector matrix will in generally not be nxn, and we do need to associate specific eigenvalues with specific eigenvectors. So one totally unambiguous data structure to pass back would be an array of objects, in order from largest eigenvalue (in abs value) to smallest, with each object having just two properties: |
Thanks for your clear explanation Glen, I now see that the results are indeed technically correct 😅, though the API and the actual values cause confusion. So, we're mixing rows/columns when extracting the vector. As a solution, I propose the following:
Does that make sense? |
@gwhitney so I like your idea of an API |
I think there are only two cases really: One being the ideal one: Other being defective matrix: I think there might be a case that you can't perform eigenvalue decomposition at all. (I do believe it might be a case of non-diagonalizable matrices. But teeechnically you still can do eigenvalue decomposition) E.x you have "two" eigenvalues being But generally having eigenvalue = 0 means that matrix is not diagonalizable |
I feel strongly that the data structure we pass back should not depend on whether the matrix is defective or not, so we should design a data structure that works in either case; the current one does not. Jos has already said he prefers a single breaking change to a non-breaking one followed by a breaking one. So I propose we pass back an array of values as we do now, with their algebraic multiplicities (so the vector will always have n entries for an nxn matrix), and then under the key 'eigenvectors' (just to change the key, so that anyone switching will immediately know there is a difference as there is no 'vectors' key), we pass back an array of plain objects, one for each independent eigenvector. Each of these plain objects has two keys: value and vector. The value property reiterates the eigenvalue for that eigenvector, but is necessary because you can't predict the geometric multiplicities from the arithmetic ones. The vector property gives a single eigenvector for that value. An alternative might be a map from eigenvalues to eigenvectors, but then it seems as though the values would be arrays of vectors, which have the confusability property that started this whole thread. I am not wedded to this proposal by any means, but I haven't thought of a better one. |
Sorry I don't think I addressed the comment of @josdejong about how this API proposal handles defective matrices. For non-defective matrices, there is one item in the |
Yes that is a very good point, let's make sure of that. The current API looks like this: math.eigs([[2, 0, 0], [0, 1, 0], [0, 0, 5]])
// {
// values: [1, 2, 5],
// vectors: [ // Remember to take the columns, not the rows!
// [ 0, 1, 0 ],
// [ 1, 0, 0 ],
// [ 0, 0, 1 ]
// ]
// }
math.eigs([[2, 1], [0, 2]])
// Currently: Error
// Result should be: a double eigenvalue 2, with only one distinct eigenvector `[1; 0]` One question: I'm not sure whether the index of the eigenvector currently correlates with the index of the correspondingeigenvalue, is that the case? I guess so? @gwhitney If we output fewer eigenvectors than eigenvalues, how can we know when eigenvector belongs to which eigenvalue? I would like an API where that is clear. What do you have in mind in that regard? Here some ideas with different output for Idea 1(I think too complex) {
values: [2, 2],
vectors: [
{
vector: [1, 0],
valueIndices: [0, 1]
}
]
} Idea 2Just repeat double eigenvalues and eigenvectors, and inform about that via a flag [
{
value: 2,
vector: [1, 0],
multiplicity: 2
},
{
value: 2,
vector: [1, 0],
multiplicity: 2
}
] Idea 3Do not repeat double eigenvalues, and inform about the multiplicity [
{
value: 2,
vector: [1, 0],
multiplicity: 2
}
] Any thoughts on this? I'm not sure if it is handiest in practice to always have a number of eigenvalues equal to the size of your matrix, or not (that would be the only reason for Idea 2). |
@josdejong you have have same eigenvalues having different eigenvectors so option 3 isn't really perfect. Maybe something like: Array[{
value: vector
vectors: vector[]
multiplicity: number
}] So: [
{
value: 2,
vectors: [[1, 0], [1, 0]],
multiplicity: 2
},
{
value: 3,
vectors: [[1, 0], [0, 1]],
multiplicity: 2
},
{
value: 1,
vectors: [[1, 0]],
multiplicity: 1
}
] Also general rule is to order eigenvalues by their real part (I believe) |
Sorry if I have not been writing clearly. Altihough I understand that you are OK with breaking changes, I think that we should try to keep the interface as close as we can. In particular, I think we should continue to return as the values key a vector always of length n of the eigenvalues. That conveys two of the four pieces of information the client may want: (A) what the eigenvalues are and (B) the algebraic multiplicity of each. Also a significant amount of time all the client wants is the values, so they should be kept easy to access. Then we need to replace the current vectors key, as it is ambiguous in defective cases, and prone to misinterpretation. The remaining info we need to convey is (C) the geometric multiplicity of each eigenvalue and (D) as many independent eigenvectors as exist. Since eigenvalues may repeat and we don't want to put eigenvectors in lists, a map from eigenvalues to eigenvectors is out. Instead we want a more general association of values and vectors, which we can pass back in a new key to make transition safer. I am proposing the key 'eigenvectors'. The value of this key would be a list of plain objects, each with a 'value' property giving the eigenvalue and a 'vector' property giving the eigenvector. An alternative would be just a list of pairs [value, vector] but that's more opaque. Anyhow, this gives you D from the vector part of each entry and C by counting how many times a given value appears. To make this concrete, suppose M has eigenvalue 4 with algebraic multiplicity 3 but geometric multiplicity 2 and independent eigenvectors [1,0,0,0] and [0,1,0,0], and also eigenvalue -2 with algebraic multiplicity 1 and eigenvector [0,0,0,1]. Then I propose eigs would return:
(but as I mention we could opt for the value of eigenvectors to be
to be more compact but also more opaque.) As to your ideas 1,2,3: As for bartekleon's suggestion, it has arrays of vectors which I think we are trying to avoid because of the structural confusion of array of vectors and matrix in mathjs, exacerbated by the fact that then the vectors look like rows when they are actually columns. Hopefully I have finally been clearer. I am fine in the end with any return value that conveys all of A,B,C,D. Within that we should strive to maximize clarity and minimize redundancy. Thanks for considering. |
Thanks Glen and Bartosz. From what I read here it looks like at least Matlab doesn't sort the returned eigenvalues. I'm not sure if there is value in sorting them. At least you can always sort them yourselves if needed. I'm OK with either sorting the output or not though if you guys think it is helpful. I like Glen's proposal a lot: {
values: [4, 4, 4, -2],
eigenvectors: [
{value: 4, vector: [1,0,0,0]},
{value: 4, vector: [0,1,0,0]},
{value: -2, vector: [0,0,0,1]}
]
} It solves the ambiguity that One last thought: using names {
values: Array | Matrix
eigenvectors: Array<{ value: number | BigNumber, vector: Array | Matrix}>
vectors: void // throws an error explaining to use eigenvectors instead
} What do you think? |
Looks good to me :) |
I do agree that the top level properties being And to your final point -- are you saying you want to, in the new version, return an object that has a getter/setter for the OK, I think we are super close --- I think Jos can just make his pronouncements on these final tiny points and then I can get started on a PR. Thanks to all! |
Ah, missed this point. I would not alter the order of the values property from whatever it is now. Even if you decide we should rename it, my goal for the PR is that there would be no change in what array of eigenvalues is produced. So if it's sorted now, it would stay sorted, and if it's not, it would stay that way, too. In my mind, the fewer collateral changes from bugfixes, the better. |
👍 sounds good. Ok let's do the following:
|
Previously, attempting to take the `eigs` of any defective matrix was doomed to fail in an attempt to solve a singular linear system. This PR detects the situation (as best as it can given the inherent numerical instability of the current methods used) and handles it. Note that in such cases, it's not possible to return a square matrix whose columns are the eigenvectors corresponding to the returned eigenvalues. In light of that fact and issue josdejong#3014, this PR also changes the return value of `eigs` so that the eigenvectors are passed back in a property `eigenvectors` which is an array of plain objects `{value: e, vector: v}`. Note that this PR makes the ancillary changes of correcting the spelling of the filename which was "realSymetric.js," and replacing the now-unnecessary auxiliary function "createArray" therein with `Array(size).fill(element)`. The rationale for performing these changes not strictly related to the issues at hand is that this file is rarely touched and with the level of maintenance hours we have at hand, it's more efficient to do these small refactorings in parallel with the actual bugfixes, which are orthogonal and so will not be obfuscated by this refactor. Note `git diff` does properly track the file name change. However, it also makes a potentially more pervasive change: in order for the numerically-sensitive algorithm to work, it changes the condition on when two very close (double) numbers are "nearlyEqual" from differing by less than DBL_EPSILON to differing by less than or equal to DBL_EPSILON. Although this may change other behaviors than the ones primarily being addressed, I believe it is an acceptable change because (a) It preserves all tests. (b) DBL_EPSILON is well below the standard config.epsilon anyway (c) I believe there are extant issues noting the odd/inconsistent behavior of nearlyEqual near 0 anyway, so I believe this will be overhauled in the future in any case. If so, the eigenvector computation will make a good test that a future nearlyEqual algorithm is working well. To be clear, the direct motivation for the change is that there are multiple cases in the eigenvector computation in which a coefficient that is "supposed" to be zero comes out to precisely DBL_EPSILON, which is fairly unsurprising given that these coefficients are produced by subtracting an eigenvalue from a diagonal entry of a matrix, which is likely to be essentially equal to that eigenvalue. As many tests of defective matrices as I could readily find by web searching have been added as unit tests (and one more in the typescript type testing). An additional case I found still fails, but in the _eigenvalue_ computation rather than the _eigenvector_ search, so that was deemed beyond the scope of this PR and has been filed as issue josdejong#3036. Resolves josdejong#2879. Resolves josdejong#2927. Resolves josdejong#3014.
Fixed in |
math.eigs
produces correct eigenvalues with incorrect eigenvectors in some cases. Note that the example in the documentation still seems to give the correct results.To Reproduce
Execute the following:
Result:
Expected:
Edit: It looks like the correct eigenvector is actually
[ -1, 1 ]
The text was updated successfully, but these errors were encountered: