-
Notifications
You must be signed in to change notification settings - Fork 22
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 vector idx abs max #290
Conversation
iburyl
commented
Sep 28, 2024
- renamed idx_abs_max -> vector_idx_abs_max
- aligned behavior for complex values with the spec (that is abs is not really abs)
- Currently if NaN is in the first position output is 0. Output will ignore NaNs in any other position and will provide an index of the max value, ignoring all other NaNs. NaNs are checked the same way as in BLAS (that is not really checked). NaN behavior is not clarified with spec. Anything beyond that implementation will have cross-board performance implications. Leave NaNs to unspecified output.
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.
Looks good to me except that I think the complex test should fail now or? Because std::abs(z)
is different from abs(z.real()) + abs(z.imag())
isn't it? Blocking to see what the tests do.
The previous used (revised in linalg adoption process) used abs(z) for complex. Now its abs(z.real()) + abs(z.imag()). The test wasn't sensitive to the change.
I pushed a change to the test which makes it sensitive to the changed behavior for complex. It wasn't before (i.e. the test passed before and with your change). |
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.
Good now. I fixed the test to be sensitive to this change.
@mhoemmen if you also approve I merge (or you can merge and we consider that approval) |
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 for the fix! : - )