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

determinant of matrix with tiny numbers #3135

Open
zanottle opened this issue Jan 23, 2024 · 6 comments · Fixed by #3139
Open

determinant of matrix with tiny numbers #3135

zanottle opened this issue Jan 23, 2024 · 6 comments · Fixed by #3139

Comments

@zanottle
Copy link

math.det([[6.123234262925839e-17,-1,1],[-0.8660253882408142,0.5,1],[-0.6495190262794495, -0.3749999701976776,1]]);
returns 0, while (the only change is in the first number from e-17 to e-10)
math.det([[6.123234262925839e-10,-1,1],[-0.8660253882408142,0.5,1],[-0.6495190262794495, -0.3749999701976776,1]]);
returns 0.433 (the correct result).

The first result is mathematically incorrect, and I am not aware of any numerical issues that could be causing this. Therefore, I believe it is a bug.

@josdejong
Copy link
Owner

Thanks for reporting. That is indeed odd.

When changing this first value to 0, the function returns the right result too:

math.det([[0,-1,1],[-0.8660253882408142,0.5,1],[-0.6495190262794495, -0.3749999701976776,1]]) 
// 0.43301264595909744

I also tried to evaluate this as bignumbers, that works correct.

Anyone able to help debug this issue?

@dvd101x
Copy link
Sponsor Collaborator

dvd101x commented Jan 25, 2024

Hi, I did run into a solution:

The function det uses isZero which is based on a util of the form:

function isZeroNumber (x) {
  return x === 0
}

That doesn't take into account epsilon, I guess that in BigNumber is a different case. In the function det we can use this isZero instead:

function isZero(x){
  return equalScalar(x, 0)  // takes epsilon into account
}

Then it works

math.det([
   [6.123234262925839e-17,-1,1],
   [-0.8660253882408142,0.5,1],
   [-0.6495190262794495, -0.3749999701976776,1]
]);
// 0.4330126459590976

So a simple change would be to use the made up function isZero just in the function det. Another solution is to change isZero to take into account epsilon so it follows:

epsilon. The minimum relative difference used to test equality between two compared values. This value is used by all relational functions. Default value is 1e-12.

If that's the case maybe some other functions should also follow that rule.

@josdejong
Copy link
Owner

O wow, thanks for debugging this issue David!

My preference has to improve isZero (and isPositive and isNegative) to use nearlyEqual under the hood, like all other relational functions like equalScalar and smaller etc.

@dvd101x
Copy link
Sponsor Collaborator

dvd101x commented Jan 25, 2024

  • Did an attempt to improve isZero which indeed resolves this issue.
  • Found a possible issue Config.epsilon doesn't seem to be working #3140 that affects nearlyEqual
  • Still thinking how to improve isPositive and isNegative, I imagine the expected behavior is for them to return false if they are close to 0.

@dvd101x
Copy link
Sponsor Collaborator

dvd101x commented Jan 25, 2024

Including a relevant discussion #2838

@josdejong
Copy link
Owner

Still thinking how to improve isPositive and isNegative, I imagine the expected behavior is for them to return false if they are close to 0.

I think the three of isZero, isPositive, and isNegative should work in a consistent way and do not overlap. A value cannot be both zero and positive at the same time, or zero and negative at the same time.

  • for all cases where isZero(x) === true, isPositive(x) and isNegative(x) should be false
  • for all cases where isPositive(x) === true, isZero(x) and isNegative(x) should be false
  • for all cases where isNegative(x) === true, isZero(x) and isPositive(x) should be false

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