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 vector_norm wrong results when order is inf #10542

Merged

Conversation

nikifaets
Copy link
Contributor

Close #10480

@nikifaets
Copy link
Contributor Author

Kudos to @suvadityamuk for deep diving into the bug, finding the root causes and proposing a solution.

@nikifaets
Copy link
Contributor Author

nikifaets commented Feb 14, 2023

I added specific test examples that cover the cases when ord is inf or -inf.

Currently, the ivy hypothesis_helpers.number_helpers.floats method does not support the allow_infinity argument that comes with the Hypothesis implementation of floats. Unless I'm missing something, this prevents us from easily sampling infs as the ord value in the test cases by. At least not by modifying the @handle_test annotation.

We can use hypothesis.strategies.floats instead of helpers.floats and use the allow_infinity argument for the ord values but this does not guarantee that an inf would be drawn every time the tests are run.

I'm open to suggestions on how to include inf examples in the generic testing method (and be sure that there are infs every time the test is run).

@iamjameskeane
Copy link
Contributor

The logic seems sound to me but might be more appropriate for @suvadityamuk to give this a check over before merging :)

Copy link
Contributor

@suvadityamuk suvadityamuk left a comment

Choose a reason for hiding this comment

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

LGTM. Checked tests, they seem to pass.

@suvadityamuk
Copy link
Contributor

@jkeane508 Please feel free to merge!

@nikifaets Thank you for your contribution!

@iamjameskeane iamjameskeane merged commit b076d58 into ivy-llc:master Feb 23, 2023
vedpatwardhan pushed a commit to vedpatwardhan/ivy that referenced this pull request Feb 26, 2023
vedpatwardhan pushed a commit to vedpatwardhan/ivy that referenced this pull request Feb 26, 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.

[Bug]: vector_norm numpy backend implementation returns wrong result for some norms
3 participants