-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Polish code for _getitem_impl_ #32868
Conversation
Thanks for your contribution! |
✅ This PR's description meets the template requirements! |
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.
LGTM
if contain_var(slice_step): | ||
inputs['StridesTensorList'] = get_new_list_tensor(slice_step) | ||
for i, dim in enumerate(slice_step): | ||
infer_flags = list(1 for i in range(len(axes))) |
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.
Would you like to get a list of 1s and the length of the list is same as len(axes)?
If so, you could simply this line as:
infer_flags = list(1 for i in axes)
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. I use infer_flags = [1] * len(axes)
to replace it for better readability.
inputs['StridesTensorList'] = get_new_list_tensor(slice_step) | ||
for i, dim in enumerate(slice_step): | ||
infer_flags = list(1 for i in range(len(axes))) | ||
from .layers import utils |
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.
Can we import at the beginning of the python file?
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.
No. Because it will cause a circular reference.
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.
OK to me
} | ||
if (use_strided_slice == True): | ||
if use_strided_slice == True: |
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.
Can we write if use_strided_slice:
?
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.
Yes. Fixed.
@@ -948,7 +884,7 @@ def get_new_list_tensor(old_list): | |||
attrs=attrs) | |||
|
|||
out = slice_out_var | |||
elif use_strided_slice == True and len(slice_axis) > 0: | |||
elif use_strided_slice == True and len(axes) > 0: |
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.
Can we replace elif use_strided_slice == True and len(axes) > 0
with elif use_strided_slice and len(axes) > 0
?
Usually if a variable x
is boolean, we write if x
instead of if x is True
or if x == True
Reference from official programming recommendations:
https://www.python.org/dev/peps/pep-0008/#programming-recommendations
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, done.
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.
LGTM
…ist/Tensor/None/Ellipsis/bool (#33528) * [cherry-pick 2.1] Polish code for _getitem_impl_ (#32868) * [cherry-pick] Polish code for setitem and getitem (#32911) * [slice getitem] Support getitem idx is Tensor or List (#33000) * [getitem] Support index is None for getitem in static mode (#33001) * [Static getitem] Support static Variable getitem for Ellipsis index (#32876) * [static getitem]Support index is list bool for getitem in static mode (#33298)
PR types
Function optimization
PR changes
APIs
Describe
As the title