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 with small numbers fix #3139

Merged
merged 30 commits into from Feb 14, 2024
Merged

Conversation

dvd101x
Copy link
Sponsor Collaborator

@dvd101x dvd101x commented Jan 25, 2024

This is an attempt to fix #3135, the issue is that isZero yields false when beeing very close to zero.

I found an issue with config.epsilon that will address separately.

@josdejong
Copy link
Owner

Thanks David. You call this PR an "attempt", but it looks like it fully addresses the rounding issue for isZero. Are there other things that you want to address here? I think it probably makes sense to address isPositive and isNegative in this same PR, so the three functions keep working in a consistent way.

@dvd101x
Copy link
Sponsor Collaborator Author

dvd101x commented Jan 31, 2024

Hi Jos, thanks for the review! I agree I shouldn't have called it an "attempt" as it indeed solves the issue in my tests.

I was hesitant that I didn't follow the suggestions exactly but I should have phrased it differently.

I agree with you, I will look into isPositive and isNegative and apply a consistent logic.

I will be back during the week with that change.

@dvd101x
Copy link
Sponsor Collaborator Author

dvd101x commented Feb 5, 2024

With this latest commit isPositive and isNegative now works as discussed including some tests.

@josdejong
Copy link
Owner

Thanks, this looks good David!

@josdejong
Copy link
Owner

This originally was to fix #3135. I think it would be good to add a unit test to verify that #3135 is solved now, or would you like to do that in a separate PR?

@josdejong
Copy link
Owner

And, reminder to myself reading #2838: we should publish this as a breaking change.

@josdejong josdejong mentioned this pull request Feb 7, 2024
8 tasks
@dvd101x
Copy link
Sponsor Collaborator Author

dvd101x commented Feb 9, 2024

I included the test for #3135
I don't know if it might be too specific.

@josdejong josdejong changed the base branch from develop to v13 February 14, 2024 16:47
@josdejong
Copy link
Owner

Looks good to me. It indeed feels like a quite specific test, but I think it's fine (we can always add more tests if needed in the future).

I'll merge this PR now in a new v13 branch.

@josdejong josdejong merged commit e1817ba into josdejong:v13 Feb 14, 2024
8 of 9 checks passed
@josdejong josdejong added this to the v13 milestone Feb 14, 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.

determinant of matrix with tiny numbers
2 participants