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

Replace array_search with floating-point filter in Eigenvector::eigenvectors #473

Conversation

Aweptimum
Copy link
Contributor

@Aweptimum Aweptimum commented Nov 6, 2023

Addresses the error found in #472

When given eigenvalues containing duplicates, the Eigenvector::eigenvectors method fails in cases where the $eigenvalues array is made up of floats. The culprit is the floating point comparison in array_search when checking for orthogonal eigenvectors. Even in the case it does find a match, it can still fail if the match is at index 0 because the body of the method is guarded by a check for !$key. If $key = 0, php evaluates !0 to true because 0 is falsey, so it can still fail to return the correct eigenvectors.

We can instead loop through the found solutions, checking if the current eigenvalue has been solved with Arithmetic::almostEqual, and then explicitly check if $key === false.

I noticed that there aren't any test cases in the EigenvectorTest that would have caught this - should I add a few?

@coveralls
Copy link

coveralls commented Nov 6, 2023

Coverage Status

coverage: 99.924% (+0.004%) from 99.92%
when pulling 163c0f9 on Aweptimum:patch-eigenvector-duplicate-solutions
into ea4f212 on markrogoyski:develop.

@markrogoyski markrogoyski changed the base branch from master to develop November 7, 2023 02:50
@markrogoyski
Copy link
Owner

Hi @Aweptimum,

Thank you for the separate PR to add this improvement.

Before I merge it, I wanted to ask if you had any unit tests cases to add that would pass with the fix and fail without it? If so I'd love to add those as regression protection against future changes.

Thanks,
Mark

@Aweptimum Aweptimum force-pushed the patch-eigenvector-duplicate-solutions branch from 7c75688 to c15924f Compare November 7, 2023 14:29
@Aweptimum
Copy link
Contributor Author

Aweptimum commented Nov 7, 2023

@markrogoyski I added a test case and rearranged the commit order so it's easier to go back and forth. I basically mimicked what happened in the QR Algorithm PR that caused failures. I put the matrices with duplicate eigenvalues in another dataprovider and added their eigenvalues into the data. In the added test, I added a really small numerical imprecision to them before passing them to the eigenvectors method. I think I may have made the offset a little too small because they don't always fail.

Edit: yeah changing from 10-15 to 10-12 makes both fail

To mimic the error found in the QR algorithm, we have to test with matrices that have duplicate eigenvalues and introduce numerical precision errors.

To do this, a list of perturbed eigenvalues is passed to the eigenvectors method. The perturbation is achieved by adding a random +/- offset on an order of magnitude smaller than the default matrix error. This should allow the math to work out fine while causing the floating point comparison to fail.
array_search seems to fail in most cases when looking for a float in an array of floats. And even if it does find a match, if the key is 0, php evaluates `!0` to true.

To find a match, we can instead loop through and compare the numbers with `Arithmetic::almostEqual` and then explicitly check if `$key === false`
@Aweptimum Aweptimum force-pushed the patch-eigenvector-duplicate-solutions branch from c15924f to 163c0f9 Compare November 7, 2023 15:36
@markrogoyski
Copy link
Owner

@markrogoyski I added a test case and rearranged the commit order so it's easier to go back and forth. I basically mimicked what happened in the QR Algorithm PR that caused failures. I put the matrices with duplicate eigenvalues in another dataprovider and added their eigenvalues into the data. In the added test, I added a really small numerical imprecision to them before passing them to the eigenvectors method. I think I may have made the offset a little too small because they don't always fail.

Edit: yeah changing from 10-15 to 10-12 makes both fail

Thanks. To be safe, due to the random element in the unit test, I ran them 1,000 times and no issues. Thanks.

@markrogoyski markrogoyski merged commit da47751 into markrogoyski:develop Nov 8, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants