drq: change view to reshape to support channels last#1367
drq: change view to reshape to support channels last#1367chunyuan-w wants to merge 2 commits intopytorch:mainfrom
Conversation
|
@xuzhao9 could you please help review this PR? Thanks! |
|
cc @jgong5 |
|
@xuzhao9 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
xuzhao9
left a comment
xuzhao9
left a comment
There was a problem hiding this comment.
LGTM, thanks for the improvement.
Just want to note that we should also add a comment in the code saying this is an update that doesn't exist in the upstream code: https://github.com/denisyarats/drq/blob/master/drq.py#L48
| self.outputs['conv%s' % (i + 1)] = conv | ||
|
|
||
| h = conv.view(conv.size(0), -1) | ||
| h = conv.reshape(conv.size(0), -1) |
There was a problem hiding this comment.
Can you add one-line comment mention that this is an improvement that doesn't exist in the upstream code (with link to the upstream code at https://github.com/denisyarats/drq/blob/master/drq.py#L48)?
There was a problem hiding this comment.
@xuzhao9 We can submit a PR to upstream too. Does it sound good to you?
There was a problem hiding this comment.
@xuzhao9 We can submit a PR to upstream too. Does it sound good to you?
Sure, if you can submit a PR to upstream and get accepted, we don't need the comment line.
There was a problem hiding this comment.
I've added one-line comment as suggested for now.
I'll submit a PR to upstream this. If it is accepted, let me submit a following-up PR later to this benchmark repo to remove the one-line comment. Is it okay?
There was a problem hiding this comment.
Yes, that shoulds perfect to me!
There was a problem hiding this comment.
FYI, I've submitted a PR to the upstream drq repo: denisyarats/drq#27.
|
For the CI failure, it seems unrelated with my changes: And it also marks that there are 2 workflows awaiting approval: @xuzhao9 could you please help on that? |
There is a pre-existing CI failure not because of this PR. Sorry but we are still working on it. |
|
@xuzhao9 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
4 participants
The current implementation of
drqdoes not support channels last input sinceviewhas constraints on the input size and stride (https://pytorch.org/docs/stable/generated/torch.Tensor.view.html):Change
viewtoreshapewhich returns a view if the shapes are compatible, and copies (equivalent to callingcontiguous()) otherwise.