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

Eigenvalues of a general matrix #2439

Closed
lpawela opened this issue Feb 22, 2022 · 5 comments · Fixed by #2445
Closed

Eigenvalues of a general matrix #2439

lpawela opened this issue Feb 22, 2022 · 5 comments · Fixed by #2445

Comments

@lpawela
Copy link

lpawela commented Feb 22, 2022

I'm trying to visualize the numerical range of a matrix on a website. For this, I need to calculate the eigenvalues of a general matrix with complex elements. This should be available since #1743 was merge. Yet, I get the following errors.

<script src="https://unpkg.com/[email protected]/lib/browser/math.js"></script>

<script type="text/javascript">
console.log(math.eigs([[1, 2], [3, 4]]));
console.log(math.eigs([[3, -2], [4, -1]]));
console.log(math.eigs([[3, -2], [math.complex(4, 2), -1]]));
</script>

The first two line work as expected, the last one results in the following error

Uncaught TypeError: No ordering relation is defined for complex numbers
    at Function.Complex, Complex (smaller.js:75:13)
    at smaller (typed-function.js:1086:85)
    at complexEigs.js:111:18
    at complexEigs.js:22:15
    at j (eigs.js:100:12)
    at Function.Array (eigs.js:51:14)
    at Object.eigs (typed-function.js:1085:85)
    at (index):1:14924
Complex, Complex	@	smaller.js:75
smaller	@	typed-function.js:1086
(anonymous)	@	complexEigs.js:111
(anonymous)	@	complexEigs.js:22
j	@	eigs.js:100
Array	@	eigs.js:51
eigs	@	typed-function.js:1085
(anonymous)	@	(index):1

Am I doing something? Did I misunderstand the nature of #1743?

@gwhitney
Copy link
Collaborator

This is definitely a bug. Thank you so much for reporting! Someone will look at it as soon as they are able, but as you can see, there is a big backlog; any suggestions as to how to address it or better yet a PR fixing it would definitely be welcome.

@lpawela
Copy link
Author

lpawela commented Feb 22, 2022

@gwhitney playing around with the console in my browser I think I found the issue. These lines

const zero = big ? bignumber(0) : cplx ? complex(0) : 0
const one = big ? bignumber(1) : cplx ? complex(1) : 1
are defined based on the type of the input. In the case I found they are complex numbers. Later, we have
let colNorm = zero
let rowNorm = zero
which makes both norms complex. Next, we add to the norms the absolute values of matrix elements
colNorm = addScalar(colNorm, c)
rowNorm = addScalar(rowNorm, c)
which keeps the norms complex. This in turn makes the variables
const rowDivRadix = divideScalar(rowNorm, radix)
const rowMulRadix = multiplyScalar(rowNorm, radix)
complex as well. Finally, we hit the offending line
while (smaller(c, rowDivRadix)) {
which results in a comparison between a real c and a complex rowDivRadix. I don't know much about javascript, and I don't have a proper environment to prepare and submit a PR, but my suggestion would be to change zero and one to a real value. Would that work, or will it break something?

gwhitney added a commit to gwhitney/mathjs that referenced this issue Feb 23, 2022
   This change fixes a typing problem in complexEigs.js in which
   real-valued norms were inadvertently being typed as complex numbers.

   Resolves josdejong#2439
@gwhitney
Copy link
Collaborator

By Jove that's it. All of the norms and radices in this part of the algorithm are intended to be real even in the case of complex entries. Fixing the types prevented the error from being thrown, and produced the correct eigenvalues, Hopefully we can get the PR above merged reasonably quickly and a new release to fix this serious bug. Thank you so much Łukasz for your help. And @josdejong let me know if there's anything in particular I should do to expedite this fix.

josdejong added a commit that referenced this issue Mar 1, 2022
This change fixes a typing problem in complexEigs.js in which
   real-valued norms were inadvertently being typed as complex numbers.

   Resolves #2439

Co-authored-by: Jos de Jong <[email protected]>
@josdejong
Copy link
Owner

wow 😳 , thanks for debugging this @lpawela and @gwhitney for working it out in a fix. Will publish the fix today or tomorrow

@josdejong
Copy link
Owner

The fix is published now in v10.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants