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

Ensure the Equals instance method for the various vector types is correct #68691

Merged
merged 5 commits into from
Apr 29, 2022

Conversation

tannergooding
Copy link
Member

This resolves #58853

@ghost
Copy link

ghost commented Apr 29, 2022

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

Issue Details

This resolves #58853

Author: tannergooding
Assignees: -
Labels:

area-System.Numerics

Milestone: -

Copy link
Member

@GrabYourPitchforks GrabYourPitchforks left a comment

Choose a reason for hiding this comment

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

LGTM. However, I would suggest adding a unit test validating that vectors populated from two different NaN patterns (if such a concept exists) still compare as equal.

Edit: We should also make sure the breaking change doc calls out the GetHashCode changes. Namely, that it's no longer stable.

@tannergooding
Copy link
Member Author

tannergooding commented Apr 29, 2022

However, I would suggest adding a unit test validating that vectors populated from two different NaN patterns (if such a concept exists) still compare as equal.

Such a concept does exist. There are actually 2^n (where n is 23 for float and 52 for double) different variations of NaN. However, all NaN follow the principle of NaN != NaN (regardless of sign or payload) and we are explicitly checking for x == x to get the nan mask.

This combined with our other NaN tests (throughout the various APIs) should be sufficient coverage of correctness in total.

@GrabYourPitchforks
Copy link
Member

The suggestion was mainly to ensure that somebody didn't try to be overly clever and "optimize" this code:

Vector128<T> result = Vector128.Equals(this, other) | ~(Vector128.Equals(this, this) | Vector128.Equals(other, other));

To this:

Vector128<T> result = Vector128.Equals(this, other) | Vector128.Equals(this.AsTSizedInt(), other.AsTSizedInt()).AsT();

Your unit tests would still pass even though this code is incorrect.

@tannergooding
Copy link
Member Author

Your unit tests would still pass even though this code is incorrect.

I'd hope no one goes touching this code and ignores the comments in place. I can add a test covering it, but will do so as part of a separate PR (building other NaNs is a bit of a pain and there is no guarantee we won't normalize them regardless, as that's allowed by the IEEE 754 spec).

@GrabYourPitchforks
Copy link
Member

I'd hope no one goes touching this code and ignores the comments in place.

I'd hope so to. Ultimately this was just a suggestion, and I think you have better judgment than I do on what people may / may not try here. Perhaps I'm a naturally paranoid person. :)

If you think this is overkill I'll defer to your decision.

@tannergooding
Copy link
Member Author

I do think its good to add as an extra safety measure. I just don't want to re-spin CI and block the bug fix from going in on it ;)

@tannergooding
Copy link
Member Author

Logged #68710 to track said test enhancement

@dakersnar
Copy link
Contributor

We're seeing a number of regressions caused by this PR on the 7.0 RC2 vs .NET 6.0 perf report, but we assume they are all by design and expected.

@tannergooding tannergooding deleted the fix-58853 branch November 11, 2022 15:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.Numerics.Vector2.Equals doesn't use float.Equals in Equals implementation
3 participants