Skip to content

Linearly interpolating upsampling fix #5927

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
Merged
ezyang merged 4 commits into from
Mar 24, 2018
Merged

Linearly interpolating upsampling fix #5927

ezyang merged 4 commits into from
Mar 24, 2018

Conversation

@ssnl
Copy link
Collaborator

@ssnl ssnl commented Mar 21, 2018 •
edited
Loading

Add align_corners option to upsampling module & functional when using linearly interpolating modes:

When align_corners=True, it uses the old original upsampling scheme, which gives visually better results,
but doesn't properly align input and output pixels, and thus cause the output vary basing on input.
This PR adds this align_corners option, and changes the default behavior to align_corners=False, with
proper warning if this option is not specified upon using nn.Upsample or nn.functional.upsample to let
be aware of this new change.
Adds tests in test_nn.py for spatial invariance when align_corners=False, and usual module tests for
align_corners=False.

The ratio is basically computed as:

ratio = align_corners ? (input_size - 1) / (output_size - 1) : input_size / output_size

And src_idx is:

if align_corners:
  src_idx = dst_idx * ratio
else:
  src_idx = (dst_idx + 0.5) * ratio - 0.5

The 0.5 are used to cast the index to location of the pixel centers.

This also makes the default upsampling behavior consistent with other DL frameworks like tf.

This solves the issue raised in #5511

cc @Dorimer

@onnxbot-worker-2 onnxbot-worker-2 mentioned this pull request Mar 21, 2018
Closed
@ssnl ssnl force-pushed the bilinear_upsampling_fix branch from 5bb6ede to dc579b5 Compare March 21, 2018 20:57
@vguzov vguzov mentioned this pull request Mar 21, 2018
Closed
@ssnl
Copy link
Collaborator Author

ssnl commented Mar 22, 2018

@pytorchbot retest this please

@ssnl ssnl force-pushed the bilinear_upsampling_fix branch 3 times, most recently from 534a7f6 to 981e339 Compare March 22, 2018 22:25
@ssnl
Copy link
Collaborator Author

ssnl commented Mar 22, 2018

@pytorchbot retest this please

@ssnl
Copy link
Collaborator Author

ssnl commented Mar 23, 2018

@Dorimer Let me know if the math formula in this PR looks okay to you.

@ezyang ezyang added the module: bc-breaking Related to a BC-breaking change label Mar 23, 2018
ezyang
ezyang reviewed Mar 23, 2018
View reviewed changes
aten/src/ATen/nn_parse.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ezyang
ezyang reviewed Mar 23, 2018
View reviewed changes
aten/src/ATen/test/basic.cpp Outdated

This comment was marked as off-topic.

Sign in to view

ezyang
ezyang reviewed Mar 23, 2018
View reviewed changes
aten/src/THCUNN/generic/SpatialUpSamplingBilinear.cu Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ezyang
ezyang reviewed Mar 23, 2018
View reviewed changes
aten/src/THCUNN/generic/SpatialUpSamplingBilinear.cu Outdated

This comment was marked as off-topic.

Sign in to view

ezyang
ezyang reviewed Mar 23, 2018
View reviewed changes
aten/src/THCUNN/generic/TemporalUpSamplingLinear.cu Outdated

This comment was marked as off-topic.

Sign in to view

ezyang
ezyang reviewed Mar 23, 2018
View reviewed changes
test/test_nn.py 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

@ezyang
Copy link
Contributor

ezyang commented Mar 23, 2018

Please come to an agreement about the formulas before merging, thanks!

@vguzov
Copy link

vguzov commented Mar 24, 2018 •
edited
Loading

@ssnl,

Let me know if the math formula in this PR looks okay to you.

Yes, I verified a formula and it looks good. However, I discovered another problem in the new behaviour – it sometimes can produce negative values: https://gist.github.com/Dorimer/899faff586f27070ce55f1cc78c69ce1
But this can be easily fixed, just replace

return scale * (dst_index + 0.5) - 0.5;

with

float src_index = scale * (dst_index + 0.5) - 0.5;
return (src_index < 0) ? 0.f : src_index;

Similar in CUDA code.

vguzov and others added 4 commits March 23, 2018 23:19
@vguzov @ssnl
Changes in bilinear upsampling
a49eab3
@ssnl
Add align_corners option to upsampling module & functional when using…
5eb2c64 
… linearly interpolating modes

When align_corners=True, it uses the old original upsampling scheme, which gives visually better results,
but doesn't properly align input and output pixels, and thus cause the output vary basing on input.
This PR adds this align_corners option, and changes the default behavior to align_corners=False, with
proper warning if this option is not specified upon using nn.Upsample or nn.functional.upsample to let
be aware of this new change.
Adds tests in test_nn.py for spatial invariance when align_corners=False, and usual module tests for
align_corners=False.
@ssnl
remove redundant checks and unnecessary variables; fix the cast
b84bc49
@ssnl
fix negative indices
c249518
@ssnl ssnl force-pushed the bilinear_upsampling_fix branch from fdafbd6 to c249518 Compare March 24, 2018 03:19
@ezyang ezyang merged commit 5d77709 into pytorch:master Mar 24, 2018
@ssnl ssnl deleted the bilinear_upsampling_fix branch March 24, 2018 16:57
@shelhamer
Copy link

shelhamer commented Mar 25, 2018

@Dorimer @ssnl for what it's worth I have been handling bilinear interpolation by ConvTranspose2d analogously to how it is done by DeconvolutionLayer in Caffe. See here for reference code: https://gist.github.com/shelhamer/2d07f3afea5d9628f530af26e3846858.

sighingnow added a commit to sighingnow/pytorch that referenced this pull request Mar 25, 2018
@sighingnow
Merge remote-tracking branch 'upstream/master' into all-any
557c55d 
* upstream/master: (663 commits)
  Fix "command not found" error in perf test (pytorch#5982)
  add pip mkl-devel to the error message when mkl is found but mkl headers are not (pytorch#5984)
  Support batch LowerCholeskyTransform (pytorch#5980)
  Linearly interpolating upsampling fix (pytorch#5927)
  Store perf numbers in S3 (pytorch#5951)
  Modidy setup docs for Windows (pytorch#5981)
  Group Normalization (pytorch#5968)
  [distributions] Implement Power transform (pytorch#5976)
  Disable TestBottleneck test_cuda on Windows (pytorch#5977)
  Fix crash when cat-ing empty cuda tensors (pytorch#5971)
  Update no_unions flag for nanopb gen and update ONNX proto files (pytorch#5972)
  Expose gradients w.r.t. input & weight for conv1d, conv2d, conv3d in Python (pytorch#5408)
  Fixed non-determinate preprocessing on DataLoader (pytorch#4640)
  add AVX2 implementation for sigmoid function (pytorch#5010)
  Implement torch.util.bottleneck (pytorch#5216)
  Remove pragma once from cpp file (pytorch#5965)
  fix mvn docs (pytorch#5967)
  Fix incorrect rendering of Tensor.index_*_ doc examples. (pytorch#5969)
  Implement range for loop in script (pytorch#5827)
  Add windows doc (pytorch#5859)
  ...

# Conflicts:
#	aten/src/TH/generic/THTensorMath.c
#	torch/_tensor_docs.py
#	torch/csrc/generic/methods/TensorCompare.cwrap
@zou3519 zou3519 mentioned this pull request Aug 28, 2018
Closed
@oawiles oawiles mentioned this pull request May 12, 2019
Closed
@ORippler ORippler mentioned this pull request Aug 7, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ezyang ezyang ezyang left review comments

Assignees

No one assigned

Labels

module: bc-breaking Related to a BC-breaking change open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ssnl @ezyang @vguzov @shelhamer