Skip to content

Conversation

@skrah
Copy link
Contributor

@skrah skrah commented Apr 25, 2019

This is a follow up PR for #19409.

@pytorchbot pytorchbot added module: cuda Related to torch.cuda, and CUDA support in general module: operators labels Apr 25, 2019
@skrah skrah added module: nn Related to torch.nn module: porting Issues related to porting TH/THNN legacy to ATen native triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module and removed module: cuda Related to torch.cuda, and CUDA support in general module: operators labels Apr 25, 2019
@skrah
Copy link
Contributor Author

skrah commented Apr 25, 2019

A more condensed version of dispatching is possible (like in FractionalMaxPool2d.cpp), but changing too much makes reviewing harder.

@skrah skrah changed the title [WIP] Follow up to adaptive_max_pool2d() port Follow up to adaptive_max_pool2d() port Apr 25, 2019
@skrah skrah requested a review from xmnlab April 25, 2019 21:18
@skrah
Copy link
Contributor Author

skrah commented Apr 25, 2019

@xmnlab I know you are busy, but it would be great if you can review when you have time.

@xmnlab
Copy link
Contributor

xmnlab commented Apr 25, 2019

@skrah sure! I will review that now :)

Copy link
Contributor

@xmnlab xmnlab left a comment

Choose a reason for hiding this comment

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

LGTM! thanks @skrah !

I have just one question related to the function naming, you use adaptive_max_pool2d_single_out_frame

the only file I found that uses single for the function name uses fractional_max_pool2d_out_single_batch_frame

but other file uses reflection_pad1d_out_loop

I am curious about if there is one standard for naming here.

@skrah
Copy link
Contributor Author

skrah commented Apr 25, 2019

Thanks @xmnlab! I don't know if there is a naming convention, but indeed FractionalMaxPool2d.cppwas my inspiration. I like the "single", and "frame" was already part of the function name before.

@xmnlab xmnlab requested a review from ezyang April 25, 2019 22:46
@skrah
Copy link
Contributor Author

skrah commented Apr 25, 2019

@ezyang I think this one should be ready now for your review, thanks!

@xmnlab
Copy link
Contributor

xmnlab commented Apr 25, 2019

@ezyang could you review this PR?

@xmnlab
Copy link
Contributor

xmnlab commented Apr 25, 2019

oh @skrah sorry .. I didn't see you have already mentioned ezyang :)

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

zdevito pushed a commit to zdevito/ATen that referenced this pull request Apr 26, 2019
Summary:
This is a follow up PR for #19409.
Pull Request resolved: pytorch/pytorch#19738

Differential Revision: D15103231

Pulled By: ezyang

fbshipit-source-id: 11c9fec641b389906b8accd22504a683331fa6ec
@facebook-github-bot
Copy link
Contributor

@ezyang merged this pull request in cb4d41a.

zhangguanheng66 pushed a commit to zhangguanheng66/pytorch that referenced this pull request May 6, 2019
Summary:
This is a follow up PR for pytorch#19409.
Pull Request resolved: pytorch#19738

Differential Revision: D15103231

Pulled By: ezyang

fbshipit-source-id: 11c9fec641b389906b8accd22504a683331fa6ec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module: nn Related to torch.nn module: porting Issues related to porting TH/THNN legacy to ATen native open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants