-
Notifications
You must be signed in to change notification settings - Fork 86
App op (atleast_1d and atleast_2d and atleast_3d) | feat (torchlib) #767
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
App op (atleast_1d and atleast_2d and atleast_3d) | feat (torchlib) #767
Conversation
def reshape_to_2d(tensor): | ||
shape = op.Shape(tensor) | ||
rank = op.Size(shape) | ||
if rank == 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.
nit: you can simplify this as below:
if rank <= 1:
tensor = op.Reshape(tensor, [1, -1])
In reshape, -1 is treated as "whatever size matches input tensor size".
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.
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.
Also applied to 3d
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.
Nice! I didn’t know about this either
), | ||
skip( | ||
"atleast_1d_single_tensor", | ||
matcher=lambda sample: isinstance(sample.input, (list, tuple)), |
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.
So we don’t take a Sequence as input? Where is this op used? How are the inputs produced?
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.
It accepts both tensor and list.
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.
Oh I see there are two variants
shape = op.Shape(tensor) | ||
rank = op.Size(shape) | ||
if rank <= 1: | ||
tensor = op.Reshape(tensor, op.Constant(value_ints=[1, -1, 1])) |
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.
You may want to double-check the downstream usage to make sure whether you want to reshape a tensor of size N to 1x1xN or Nx1x1 ... the above reshapes it to 1xNx1 .... which may be okay, but just wondering.
op.SequenceMap comes to the rescue for element iteration cases. (Maybe it's just me that didn't know this...)
This PR helps complete #762