fix: broadcasting issue in compositional set_item

[mattbarrett98]

This fixes the following for tf and jax (already works for torch/numpy):

import ivy

query = (slice(None, None, None), slice(None, None, None), slice(None, None, None), 1)

ivy.set_tensorflow_backend()
x = ivy.ones((1, 3, 3, 3))
val = ivy.zeros((3, 1))
x[query] = val
print(x)

ivy.set_jax_backend()
x = ivy.ones((1, 3, 3, 3))
val = ivy.zeros((3, 1))
x[query] = val
print(x)

I suspect the expanding dims logic maybe isn't necessary before the broadcasting but not sure.

Any ideas on if it's possibe with hypothesis to add a very specifc example to a test, like my example. I imagine it being a lot more complicated to get examples like this to appear in a property based way, but i don't think property based testing is really that important here and just testing one specific example would be sufficient. Is this possible @vedpatwardhan @Ishticode ?

[vedpatwardhan]

Any ideas on if it's possibe with hypothesis to add a very specifc example to a test, like my example. I imagine it being a lot more complicated to get examples like this to appear in a property based way, but i don't think property based testing is really that important here and just testing one specific example would be sufficient. Is this possible @vedpatwardhan @Ishticode ?

One thing we can do is use hypothesis.example which can be used alongside @given in our tests. Could you please check if this works with the current tests as well? Also, what do you think should be the criteria to decide when to add a strategy and when to add an explicit example for a particular example? I'll add that as an explanation to the docs as well.
As for the changes to set_item, I'll leave those with @Ishticode to review as he's already working on some other issues with get_item and set_item. Thanks @mattbarrett98 😄

[Ishticode]

looks good

[mattbarrett98]

One thing we can do is use hypothesis.example which can be used alongside @given in our tests. Could you please check if this works with the current tests as well? Also, what do you think should be the criteria to decide when to add a strategy and when to add an explicit example for a particular example? I'll add that as an explanation to the docs as well. As for the changes to set_item, I'll leave those with @Ishticode to review as he's already working on some other issues with get_item and set_item. Thanks @mattbarrett98 😄

Thanks for the suggestion @vedpatwardhan, i've added a hypothesis.example as you said and it's correctly failing without my fix to set_item.

As for criteria i'm not too sure. I think in this scenario it's just far simpler to add an example, and also I think accomplishes everything a full-blown strategy approach would. Since the problem in question here was some broadcasting issue, which occurs in a fairly specific situation of the shapes being a certain way, with the dtypes and values of the arrays not being relevant at all here.

This way seems far more efficient to me, as trying to change the strategies to include examples like this would probably expand the search space more than necessary for this specific example. Whereas with this approach the exact problematic example will be tested each time, for a more efficient, deterministic CI.

Let me know if i'm good to merge @vedpatwardhan 😁

[vedpatwardhan]

Thanks for the suggestion @vedpatwardhan, i've added a hypothesis.example as you said and it's correctly failing without my fix to set_item.

As for criteria i'm not too sure. I think in this scenario it's just far simpler to add an example, and also I think accomplishes everything a full-blown strategy approach would. Since the problem in question here was some broadcasting issue, which occurs in a fairly specific situation of the shapes being a certain way, with the dtypes and values of the arrays not being relevant at all here.

This way seems far more efficient to me, as trying to change the strategies to include examples like this would probably expand the search space more than necessary for this specific example. Whereas with this approach the exact problematic example will be tested each time, for a more efficient, deterministic CI.

Let me know if i'm good to merge @vedpatwardhan 😁

Yep sounds good! I'll create a task for adding a new decorator that uses example so that one doesn't need to specify all the test flags that should be used in the test even if they are the default values of those flags.
There's a few errors in the tests such as this, this, etc., could you please confirm if the test failures aren't related to your changes? Thanks @mattbarrett98 😄

[mattbarrett98]

Thanks for the suggestion @vedpatwardhan, i've added a hypothesis.example as you said and it's correctly failing without my fix to set_item.
As for criteria i'm not too sure. I think in this scenario it's just far simpler to add an example, and also I think accomplishes everything a full-blown strategy approach would. Since the problem in question here was some broadcasting issue, which occurs in a fairly specific situation of the shapes being a certain way, with the dtypes and values of the arrays not being relevant at all here.
This way seems far more efficient to me, as trying to change the strategies to include examples like this would probably expand the search space more than necessary for this specific example. Whereas with this approach the exact problematic example will be tested each time, for a more efficient, deterministic CI.
Let me know if i'm good to merge @vedpatwardhan 😁

Yep sounds good! I'll create a task for adding a new decorator that uses example so that one doesn't need to specify all the test flags that should be used in the test even if they are the default values of those flags. There's a few errors in the tests such as this, this, etc., could you please confirm if the test failures aren't related to your changes? Thanks @mattbarrett98 😄

Sounds good 😁 I think you linked the same failure twice, but i checked this one and it is indeed unrelated and fails with the same without my change (although it does look very related). Also went through the some other failures that look set_item related and none were due to this change. Think we're good to merge @vedpatwardhan ?

[vedpatwardhan]

Sounds good 😁 I think you linked the same failure twice, but i checked this one and it is indeed unrelated and fails with the same without my change (although it does look very related). Also went through the some other failures that look set_item related and none were due to this change. Think we're good to merge @vedpatwardhan ?

Yep sure, let's get the PR merged, thanks @mattbarrett98 😄