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

FIX: Fixes dot() returning NaN. #26

Merged
merged 1 commit into from
Sep 26, 2022

Conversation

SolarLune
Copy link
Contributor

Here's a PR to fix angle() sometimes returning NaN with specific long-running vectors; this is probably due to floating-point inaccuracies.

As an example, without this PR, this:

v := vector.Vector{-0.9040721420170615, 0, 0.4273798802338293}
fmt.Println(v.Angle(v))

returns NaN instead of 0 (and similarly NaN instead of pi if the vector were to be tested against its opposite).

Copy link
Owner

@quartercastle quartercastle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing this @SolarLune, can i get you to add a unit test to verify this case, just to ensure it isn't reintroduced in the future?

This seems to be due to floating point inaccuracies.
@SolarLune
Copy link
Contributor Author

Whoops, sorry about that - OK, I force updated my master branch so the current PR now includes a simple dot NaN test.

Copy link
Owner

@quartercastle quartercastle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, looks good! 👍

@SolarLune
Copy link
Contributor Author

Excellent - and sorry for the confusing title / description, haha. This issue affects both dot() and angle().

@quartercastle
Copy link
Owner

Yeah, i figured that out after seeing the code. Once again thanks for the contribution! :)

@quartercastle quartercastle merged commit 007106a into quartercastle:main Sep 26, 2022
quartercastle added a commit that referenced this pull request Jul 22, 2023
quartercastle added a commit that referenced this pull request Jul 22, 2023
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.

2 participants