fix: numpy indexing in tf backend getitem

[mattbarrett98]

No description provided.

[vedpatwardhan]

lgtm! Feel free to merge the PR once the speedup is tested, thanks @mattbarrett98 😄
(The failures in the CI seem unrelated to the changes made)

[vedpatwardhan]

probably let's also create the set_item PR if we can do something similar there, would be helpful for doing the benchmarks 😄

[mattbarrett98]

lgtm! Feel free to merge the PR once the speedup is tested, thanks @mattbarrett98 😄 (The failures in the CI seem unrelated to the changes made)

haris said it seems to be working for the problematic models so all good there. How does the ci work, how come it is raising these errors if they aren't related? (although some of them definitely do seem to be unrelated)

[mattbarrett98]

probably let's also create the set_item PR if we can do something similar there, would be helpful for doing the benchmarks 😄

seems it doesn't do the same for mutations, but this isn't a blocker for the demos anyway so it's okay (for now)

[vedpatwardhan]

I think the main issue is the way mapping is computed across tests. So when we try to compute the mapping with coverage and we run all the tests (every saturday), basically many of the tests eventually end up using get_item when they call ivy.Array.__getitem__. So seems like there's quite a few tests that are considered related to the get_item function and so all of them get triggered. But given that many of them have already been failing, we don't need to worry about them too much. We only need to be concerned about failures marked as (main: pass) because those are the ones that pass on main and set_item seems to be one of them 😅

[vedpatwardhan]

probably let's also create the set_item PR if we can do something similar there, would be helpful for doing the benchmarks 😄

seems it doesn't do the same for mutations, but this isn't a blocker for the demos anyway so it's okay (for now)

Yeah sure, but probably we can do something similar to this by just doing the getitem call instead of this line (where we have to convert the array to numpy before applying the get_item) and that should help just get the _parse_query helper reverted back to what it was before my commit which should get set_item passing as well. I know that it isn't as high priority but if it's a quick thing to do probably we can do that too, either in this PR or in a different PR. Actually @Ishticode could you please create this second PR? Let's merge this one for now in that case. Thanks @mattbarrett98, @Ishticode 😄

[mattbarrett98]

I think the main issue is the way mapping is computed across tests. So when we try to compute the mapping with coverage and we run all the tests (every saturday), basically many of the tests eventually end up using get_item when they call ivy.Array.__getitem__. So seems like there's quite a few tests that are considered related to the get_item function and so all of them get triggered. But given that many of them have already been failing, we don't need to worry about them too much. We only need to be concerned about failures marked as (main: pass) because those are the ones that pass on main and set_item seems to be one of them 😅

i see, so the only one listed with main: pass is the set_item one you linked. however i just tried this on main and it does fail with the same, and this is a failure in the torch backend anyway which can't be related to these changes

[mattbarrett98]

probably let's also create the set_item PR if we can do something similar there, would be helpful for doing the benchmarks 😄

seems it doesn't do the same for mutations, but this isn't a blocker for the demos anyway so it's okay (for now)

Yeah sure, but probably we can do something similar to this by just doing the getitem call instead of this line (where we have to convert the array to numpy before applying the get_item) and that should help just get the _parse_query helper reverted back to what it was before my commit which should get set_item passing as well. I know that it isn't as high priority but if it's a quick thing to do probably we can do that too, either in this PR or in a different PR. Actually @Ishticode could you please create this second PR? Let's merge this one for now in that case. Thanks @mattbarrett98, @Ishticode 😄

makes sense, i'd say let's give it at least a week or 2 so we can gauge how well the new solution is working