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

Add failing RREF Case #475

Closed
wants to merge 2 commits into from
Closed

Conversation

Aweptimum
Copy link
Contributor

As laid out in #472, this adds a case where the calculated rref is different from the expected one

@markrogoyski
Copy link
Owner

Hi @Aweptimum,

Thank you for opening a ticket and adding the test case. I'll take a look.

Mark

@Aweptimum
Copy link
Contributor Author

Aweptimum commented Nov 9, 2023

It seems the issue is that when working with the two 0.0006114 entries as pivots, the row operations on the two lower-right entries of -3.738869*10-7 result in 4.356*10-17. Since it's below the margin of error, both of the lower diagonal entries are zeroed out. It would give the correct result otherwise.

@markrogoyski
Copy link
Owner

Have you tried manually setting setError() to something appropriate and then computing it?

@Aweptimum
Copy link
Contributor Author

Yes, if the error is set to 10-20 it passes.

@markrogoyski
Copy link
Owner

I'm inclined to think this isn't an issue, and just a natural result of numerical computations that involve divisions and comparisons. As soon as you divide numbers, you never really know what they are anymore due to floating-point precision errors. How do you determine if a very small number is intended, or zero? The library uses an arbitrary error threshold that works for most scenarios, but the option is there to override that when you know you are working with really small numbers.

How about we add this test case as a special case setting the error threshold and then call this one closed? Thoughts?

@Aweptimum
Copy link
Contributor Author

Aweptimum commented Nov 9, 2023

I think there's something that can be done to improve the stability because the scaled up matrix passes. It's like the error needs to account for the relative magnitudes of the numbers in the matrix. Trouble is I'm having a hard time finding anything on improving the stability of gaussian elimination. But it's hard as a user to come up against a problem like this when the original given matrix only contains numbers on the order of 101 to 103

@Beakerboy
Copy link
Contributor

Beakerboy commented Nov 9, 2023

If all the elements are rational numbers, the RationalNumber class could be used. It would eliminate floating point errors. You can load an ObjectMatrix with RationalNumber objects, and the rref math should still work.

edit: Nevermind, I thought the method was abstracted to work on math Objects as well a built-in numeric types. If the RowDivide() and rowAdd() methods are implemented in ObjectMatrix, and the gaussianEliminiation method was abstracted more, it would work.

The matrix with the bigger magnitude passes, the same matrix scaled by its largest eigenvalue fails. Yet they should have the same rref.
Since we're technically manipulating a linear system, it seems that the property of the final result remaining the same despite scaling applies. If scaling by the inverse of the smallest element, the matrix error becomes relative to the magnitudes of the numbers rather than an absolute quantity.

This intuition seems true because all of the tests pass
@Aweptimum
Copy link
Contributor Author

Aweptimum commented Nov 13, 2023

After staring at it harder last week, I realized that the rref might be right because it came from a result of A - λI. The λ in question is in the eigenvalue array twice so it should have 2 solutions. Which means there should be 2 zero rows, right?

So yeah you can probably just close this. There's an issue elsewhere

@Aweptimum Aweptimum closed this Jan 19, 2024
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