Skip to content
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

Added grid_sample backward batch rule #284

Merged
merged 5 commits into from
Dec 2, 2021

Conversation

vfdev-5
Copy link
Contributor

@vfdev-5 vfdev-5 commented Nov 22, 2021

Description:

  • Added grid_sample backward batch rule: CPU and CUDA
  • Updated tests
  • Updated codegen/codegen_outofplacebatching.py to be able to parse methods with std::array in signature
  • Regenerated functorch/csrc/OutOfPlacePlumbing.cpp

Notes:
I had to expand on dim 0 in most of the cases and could not use
tricks like in forward pass when batch dim is merged either with channel or H_out
due to wrong grid grads in these cases

Related to #240

Description:
- Added grid_sample backward batch rule: CPU and CUDA
- Updated tests

Notes:
I had to expand on dim 0 in most of the cases and could not use
tricks like in forward pass when batch dim is merged either with channel or H_out
due to wrong grid grads in these cases
Comment on lines +649 to +650
VMAP_SUPPORT("grid_sampler_3d_backward", GRID_SAMPLE_BW_BATCH_RULE(grid_sampler_3d_backward));
VMAP_SUPPORT("cudnn_grid_sampler_backward", CUDNN_GRID_SAMPLE_BW_BATCH_RULE(cudnn_grid_sampler_backward));
Copy link
Contributor

Choose a reason for hiding this comment

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

Do both these get exercised in the tests?

Copy link
Contributor Author

@vfdev-5 vfdev-5 Nov 30, 2021

Choose a reason for hiding this comment

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

Yes, cudnn_grid_sampler_backward is tested by test/test_ops.py::TestOperatorsCUDA::test_vmapvjp_has_batch_rule_nn_functional_grid_sample_cuda_float32
and grid_sampler_3d_backward is tested by one of test/test_ops.py::TestOperatorsCPU::test_vmapvjp_has_batch_rule_nn_functional_grid_sample_cpu_float32

Copy link
Contributor

@zou3519 zou3519 left a comment

Choose a reason for hiding this comment

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

This looks good to me. I had a suggestion for how to reduce the number of cases and we should rebase and regenerate OutOfPlacePlumbing.cpp (I added a change to codegen/codegen_outofplacebatching.py).

functorch/csrc/BatchRulesModules.cpp Outdated Show resolved Hide resolved
functorch/csrc/BatchRulesModules.cpp Outdated Show resolved Hide resolved
functorch/csrc/BatchRulesModules.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@zou3519 zou3519 left a comment

Choose a reason for hiding this comment

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

thanks!

@zou3519
Copy link
Contributor

zou3519 commented Nov 30, 2021

Hmm, tests are failing with build issue but I'm not sure that they're related

@samdow
Copy link
Contributor

samdow commented Nov 30, 2021

Test is failing because we're getting an unexpected success from test_vmapvjp_nn_functional_pad_circular_cpu_float32

I'm also not sure if this is related but just to flag if it is

@vfdev-5
Copy link
Contributor Author

vfdev-5 commented Nov 30, 2021

There is a cuda test failing at build due to pytorch 1.7.1 installation by conda (somehow), https://app.circleci.com/pipelines/github/pytorch/functorch/798/workflows/e842f064-bc17-4925-b330-88f0aa0c94a1/jobs/4776?invite=true#step-103-333

@samdow
Copy link
Contributor

samdow commented Nov 30, 2021

There is a cuda test failing at build due to pytorch 1.7.1 installation by conda (somehow), https://app.circleci.com/pipelines/github/pytorch/functorch/798/workflows/e842f064-bc17-4925-b330-88f0aa0c94a1/jobs/4776?invite=true#step-103-333

Just reran the tests (sorry that it reran all of them...I mean to only do that one). It looks like that just hit a weird race condition or something funky happened with the checkout since the root errors were a bunch of "file not found"s. I think the CUDA test failures should be similar across the 3 versions after this

@vfdev-5
Copy link
Contributor Author

vfdev-5 commented Dec 1, 2021

I synced the PR to the main branch as main is passing now and let's see if something still breaks

@zou3519 zou3519 merged commit 9bc5f1b into pytorch:main Dec 2, 2021
@vfdev-5 vfdev-5 deleted the add-grid_sample-bw branch December 2, 2021 20:31
zou3519 pushed a commit to zou3519/pytorch that referenced this pull request Jul 20, 2022
)

* Added grid_sample backward batch rule
Description:
- Added grid_sample backward batch rule: CPU and CUDA
- Updated tests

Notes:
I had to expand on dim 0 in most of the cases and could not use
tricks like in forward pass when batch dim is merged either with channel or H_out
due to wrong grid grads in these cases

* Code updates according to the review

* Updated OutOfPlacePlumbing.cpp to the latest pytorch
bigfootjon pushed a commit to pytorch/pytorch that referenced this pull request Jul 21, 2022
)

* Added grid_sample backward batch rule
Description:
- Added grid_sample backward batch rule: CPU and CUDA
- Updated tests

Notes:
I had to expand on dim 0 in most of the cases and could not use
tricks like in forward pass when batch dim is merged either with channel or H_out
due to wrong grid grads in these cases

* Code updates according to the review

* Updated OutOfPlacePlumbing.cpp to the latest pytorch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants