-
Notifications
You must be signed in to change notification settings - Fork 35
FIX: compare to epsilon during rref #90
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
base: dev
Are you sure you want to change the base?
FIX: compare to epsilon during rref #90
Conversation
src/structure/matrix.rs
Outdated
| /// | ||
| /// Implementation of [RosettaCode](https://rosettacode.org/wiki/Reduced_row_echelon_form) | ||
| fn rref(&self) -> Matrix { | ||
| let epsilon = 1e-10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This particular value of epsilon feels a bit arbitrary when applied universally here.
Would you be interested in considering floating-point constants like machine epsilon or smallest positive normal number? I also assume the choice of a particular threshold constant should be justified formally if possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @djmaxus, but if we use f64::EPSILON, then since it's too strict, so it may not be practical. How's your opinion? @developing-human @djmaxus.
Furthermore, I think it's better to use relative tolerance as follows:
let max_abs = self.data.iter().fold(0f64, |acc, &x| acc.max(x.abs()));
let epsilon = (max_abs * 1e-12).max(1e-15);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good feedback, thank you.
I had been thinking of having the function accept a value for epsilon (sympy does similar) but this would be a breaking change.
I think computing epsilon based on the values in the matrix is a good solution. I confirmed it works for my use case and pushed an update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @developing-human and @Axect
|
Happy new year @Axect. Do you need anything else from me to get this merged? |
I found a matrix that was numerically unstable while calculating reduced row echelon form. I've updated the comparisons to zero so they instead compare to epsilon, which has been more stable for my use case.
I added a test case for the unstable scenario, as well as a more basic test since one wasn't present yet for rref.
Updating numerical crates is outside my usual wheelhouse, so I'm very open to feedback here!