Skip to content

drq: change view to reshape to support channels last #1367

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.

Sign up for GitHub

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

Closed
chunyuan-w wants to merge 2 commits into from
Closed

drq: change view to reshape to support channels last #1367

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2a64a74
drq: change view to reshape to support channels last
chunyuan-w Jan 12, 2023
9ae34ad
add comment
chunyuan-w Jan 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion torchbenchmark/models/drq/drq.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@ def forward_conv(self, obs):
conv = torch.relu(self.convs[i](conv))
self.outputs['conv%s' % (i + 1)] = conv

h = conv.view(conv.size(0), -1)
# Changed view to reshape here to support channels last input
# TODO: upstream this change to https://github.com/denisyarats/drq/blob/master/drq.py#L48
h = conv.reshape(conv.size(0), -1)
Copy link
Contributor

@xuzhao9 xuzhao9 Jan 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link

@jgong5 jgong5 Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xuzhao9 We can submit a PR to upstream too. Does it sound good to you?

Copy link
Contributor

@xuzhao9 xuzhao9 Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

@chunyuan-w chunyuan-w Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@xuzhao9 xuzhao9 Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that shoulds perfect to me!

Copy link
Contributor Author

@chunyuan-w chunyuan-w Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, I've submitted a PR to the upstream drq repo: denisyarats/drq#27.

return h

def forward(self, obs, detach=False):
Expand Down