Skip to content

interpolate #14123

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

Jump to bottom
Closed
eellison wants to merge 13 commits into from
Closed

interpolate #14123

eellison wants to merge 13 commits into from

Conversation

@eellison
Copy link
Contributor

@eellison eellison commented Nov 16, 2018

Add support for interpolate and upsampling in weak_script mode.

Because the function parameters are overloaded, i had to add it as a builtin op. For interpolate:
size can be ?int | int[]?, and scale_factor can be ?float | float[]?. Every combination of the two parameters needs to be supported.

The same logic applies for upsample_nearest, upsample_bilinear, and upsample.

There are a few fixes that I came to along the way.

@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Nov 16, 2018
wanchaol
wanchaol reviewed Nov 16, 2018
View reviewed changes
aten/src/ATen/core/jit_type.h Outdated

This comment was marked as off-topic.

Sign in to view

test/test_jit.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/register_prim_ops.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

driazati
driazati reviewed Nov 16, 2018
View reviewed changes
torch/nn/modules/upsampling.py Outdated

This comment was marked as off-topic.

Sign in to view

driazati
driazati reviewed Nov 16, 2018
View reviewed changes
test/test_jit.py Outdated

This comment was marked as off-topic.

Sign in to view

driazati
driazati reviewed Nov 16, 2018
View reviewed changes
torch/csrc/jit/register_prim_ops.cpp Outdated

This comment was marked as off-topic.

Sign in to view

zou3519
zou3519 reviewed Nov 26, 2018
View reviewed changes
torch/csrc/jit/register_prim_ops.cpp Outdated
Copy link
Contributor

@zou3519 zou3519 Nov 26, 2018

Choose a reason for hiding this comment

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

Is there a reason why this isn't written as a native function?

Copy link
Contributor Author

@eellison eellison Nov 26, 2018

Choose a reason for hiding this comment

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

If it were a native function the function would have to have 4 different variants, one for each combination of size & scale_factor. Plus doing it this way makes the logic in c++ and the existing python function 1-1.

It is kind of annoying case. It might be better to move it to builtin_functions.cpp, but it would require having 4 variants and/or adding some overhead to support them, plus there are other features that are not yet implemented in builtin_functions that this would require.

@eellison eellison force-pushed the interpolate branch 3 times, most recently from c3a07b1 to 8b59a67 Compare November 29, 2018 23:09
driazati
driazati reviewed Nov 30, 2018
View reviewed changes
test/test_jit.py Outdated
Copy link
Contributor

@driazati driazati Nov 30, 2018

Choose a reason for hiding this comment

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

nn_module_tests isn't used anywhere anymore, so these could all be deleted

driazati
driazati approved these changes Nov 30, 2018
View reviewed changes
Copy link
Contributor

@driazati driazati left a comment

Choose a reason for hiding this comment

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

Looks good for now to unblock interpolate, we should follow up for a better solution

facebook-github-bot
facebook-github-bot reviewed Nov 30, 2018
View reviewed changes
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.

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

@eellison eellison force-pushed the interpolate branch 2 times, most recently from f8025af to 34016f4 Compare December 4, 2018 00:05
facebook-github-bot
facebook-github-bot reviewed Dec 4, 2018
View reviewed changes
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.

@eellison has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot
facebook-github-bot reviewed Dec 4, 2018
View reviewed changes
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.

@eellison has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot
facebook-github-bot reviewed Dec 4, 2018
View reviewed changes
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.

@eellison has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot
facebook-github-bot reviewed Dec 4, 2018
View reviewed changes
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.

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

ssnl
ssnl reviewed Mar 12, 2019
View reviewed changes
torch/nn/modules/upsampling.py

@weak_script_method
def forward(self, input):
warnings.warn("nn.{} is deprecated. Use nn.functional.interpolate instead.".format(self.name))
Copy link
Collaborator

@ssnl ssnl Mar 12, 2019

Choose a reason for hiding this comment

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

hey this undid #11568 !!!!

@ssnl ssnl mentioned this pull request Mar 12, 2019
Closed
@ailzhang ailzhang mentioned this pull request Mar 12, 2019
Closed
facebook-github-bot pushed a commit that referenced this pull request Mar 12, 2019
@facebook-github-bot
remove warning for upsample code (#17921)
3e00f79 
Summary:
IIRC we decided to remove warning in code in #11568. This got reverted accidentally in #14123.
Pull Request resolved: #17921

Differential Revision: D14422811

Pulled By: ailzhang

fbshipit-source-id: 7067264bd1d3e3b7861d29e18ade2969ed705ca1
@ezyang ezyang added the merged label Jun 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zou3519 zou3519 zou3519 left review comments

@ssnl ssnl ssnl left review comments

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@wanchaol wanchaol wanchaol left review comments

+1 more reviewer

@driazati driazati driazati approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

oncall: jit Add this issue/PR to JIT oncall triage queue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@eellison @zou3519 @ssnl @facebook-github-bot @driazati @wanchaol @ezyang