-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Relaxing type requirements for slice_like op #14097
Conversation
@mxnet-label-bot add [Operator, pr-awaiting-review] |
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 a lot for the contribution!
2 comments -
Could you rephrase this in your PR description - "Currently the type of the second shape_from
argument in slice_like
op needs to be the same as the first argument, even though only its shape is used."
I didn't understand what you mean by "even though only "its" shape is used" Do you mean the shape of the first argument is used?
And maybe consider generalizing the unittest initialization (unless it is essential)
@@ -2515,6 +2515,20 @@ def test_slice_like(): | |||
assert_allclose(xx, xgrad.asnumpy()) | |||
assert_allclose(xgrad1.asnumpy(), mx.nd.zeros_like(xgrad1).asnumpy()) | |||
|
|||
@with_seed() | |||
def test_slice_like_different_types(): | |||
x = [[ 1., 2., 3., 4.], |
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.
Though this example looks good, I am afraid it is specific. Do you mind using randomized shape, size and NDarray initalization to ensure this is generic?
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.
Those parameters do not matter for this test (and they are already tested in the dedicated test of slice_like
op functionality) - the values for x
and y
were taken from the example from slice_like
documentation. The only thing that matters is the type of y
in this test. Before this PR this code would not work - it would throw an exception during type inference. With this PR, this code works (as it should).
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.
Alright. Makes sense. Thanks for the explanation.
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
* Relaxing types for slice_like op * Added test * Fix typo in test * Fix lint
* Relaxing types for slice_like op * Added test * Fix typo in test * Fix lint
* Relaxing types for slice_like op * Added test * Fix typo in test * Fix lint
* Relaxing types for slice_like op * Added test * Fix typo in test * Fix lint
* Relaxing types for slice_like op * Added test * Fix typo in test * Fix lint
Description
Currently the type of the second argument in
slice_like
op (shape_from
) needs to be the same as the first argument, even though only its (second argument's) shape is used. This PR relaxes that soslice_like
accepts any type for theshape_from
argument.Checklist
Essentials
Please feel free to remove inapplicable items for your PR.