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: use of ivy.Shape in conditions #28224

Merged
merged 3 commits into from
Feb 9, 2024
Merged

Conversation

mattbarrett98
Copy link
Contributor

@mattbarrett98 mattbarrett98 commented Feb 8, 2024

fixing the __bool__ method of ivy.Shape. problem is lists/tuples don't have a __bool__ method, instead we need to call bool() on them for it to work. this breaks the use of an ivy.Shape object in conditions, and added a test which shows what this fixes @vedpatwardhan

Copy link
Contributor

@vedpatwardhan vedpatwardhan left a comment

Choose a reason for hiding this comment

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

lgtm! Feel free to merge, thanks @mattbarrett98 😄

@mattbarrett98
Copy link
Contributor Author

@vedpatwardhan there is a recent pr from @fnhirwa on the same method: 9789ea5. I've overridden with my change as I don't believe this was the right fix. The method now tries to extract the first element out of the shape, and check the truth value of this element, which i don't think is quite right.

I think it would coincidentally give the right answer most of the time, but one example where it fails is the shape of a 0 dimensional array would be ivy.Shape(()) ie an empty tuple. Trying to extract the 0th element from this would fail.

@fnhirwa
Copy link
Contributor

fnhirwa commented Feb 9, 2024

@vedpatwardhan there is a recent pr from @fnhirwa on the same method: 9789ea5. I've overridden with my change as I don't believe this was the right fix. The method now tries to extract the first element out of the shape, and check the truth value of this element, which i don't think is quite right.

I think it would coincidentally give the right answer most of the time, but one example where it fails is the shape of a 0 dimensional array would be ivy.Shape(()) ie an empty tuple. Trying to extract the 0th element from this would fail.

Hey @mattbarrett98 this makes sense, I'm going to revert this commit🤞

@vedpatwardhan
Copy link
Contributor

@vedpatwardhan there is a recent pr from @fnhirwa on the same method: 9789ea5. I've overridden with my change as I don't believe this was the right fix. The method now tries to extract the first element out of the shape, and check the truth value of this element, which i don't think is quite right.

I think it would coincidentally give the right answer most of the time, but one example where it fails is the shape of a 0 dimensional array would be ivy.Shape(()) ie an empty tuple. Trying to extract the 0th element from this would fail.

Yeah makes sense, thanks for pointing that out @mattbarrett98 😄

@mattbarrett98 mattbarrett98 merged commit 47926c6 into ivy-llc:main Feb 9, 2024
98 of 121 checks passed
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.

4 participants