Roots of polynomials with Complex Coefficients #1143
Conversation
…cients, based on existing .Roots() function in Polynomial class
src/Numerics/FindRoots.cs
Outdated
| /// <summary> | ||
| /// Find all roots of a polynomial with complex coefficients by calculating the characteristic polynomial of the companion matrix | ||
| /// </summary> | ||
| /// <param name="coefficients">The coefficients of the polynomial in ascending order, e.g. new Complex[] {new (5, 0), new (0, 2) new (2,3)} = "5 + 2i x^1 + (2 + 3i) x^2"</param> |
There was a problem hiding this comment.
new (0, 2) new (2,3) -> new (0, 2), new (2,3) (a comma is missing)
src/Numerics/FindRoots.cs
Outdated
| int n = coefficients.Length; | ||
| if (n < 2) | ||
| { | ||
| return null; |
There was a problem hiding this comment.
Wouldn't it be better to return an empty array/list instead of null?
jkalias
left a comment
jkalias
left a comment
There was a problem hiding this comment.
Can you please add some unit tests covering various tests cases?
I'll get on that today and try my best! Sorry for any inconvenience, this is my first real pull request. |
…tead of null. Added unit test for the function.
No worries, we all start somewhere :) |
| DenseMatrix A = new DenseMatrix(n); | ||
|
|
||
| A.SetSubMatrix(1, 0, A0); | ||
| A.SetRow(0, p.Reverse().ToArray()); |
There was a problem hiding this comment.
Shouldn't this be the last column, based on this wiki article?
https://en.wikipedia.org/wiki/Companion_matrix
Unless you decide to set the submatrix from the second column and first row. Or maybe I am confused :)
There was a problem hiding this comment.
So to be entirely honest, I don't know the theory. This is based entirely on the existing .Roots() property in the Polynomial class, which uses Evd to approximate the roots when the polynomial is higher than degree 3, if I'm not mistaken. I monkeyed around with the complex version of the DenseMatrix class and found out everything still works.
I'll triple check that this is producing accurate roots, but it seemed to pass the unit test w/o issue.
| var r1 = FindRoots.Roots( new Complex[] { c1[0] }); | ||
| Assert.AreEqual(Array.Empty<Complex>(), r1); | ||
|
|
||
| var r2 = FindRoots.Roots(new Complex[] { c1[0], c1[1] }); |
There was a problem hiding this comment.
This is supposed to be a first order polynomial, right? So we should have a single root, shouldn't we?
There was a problem hiding this comment.
So all I know is that when the Polynomial.Roots() property is called and it uses Evd to approximate, it doesn't accept first order polynomials. I wanted to keep everything as close to existing code as possible, but I can definitely add a case for first order polynomials that bypasses the Evd approximation. I could also include quadratic and cubic cases if that's desired.
| } | ||
|
|
||
| [Test] | ||
| public void RootsTest() |
There was a problem hiding this comment.
Please separate the test cases in separate test functions. This way it's much easier to follow what the edge cases are and what the normal/expected behavior is.
There was a problem hiding this comment.
Ok! I'll look around the existing test code more for good examples of what to emulate. I have an idea of what you mean, but it doesn't hurt to keep things as similar to the existing library as possible.
…ase for less than two coefficients provided
2 participants
Added function to FindRoots class for polynomials with complex coefficients, based on existing .Roots() function in Polynomial class.
I hope I did this right!