Skip to content

Conversation

@ssnl
Copy link
Collaborator

@ssnl ssnl commented Jul 31, 2018

closes #9702 .

cc @jph00

Commit structure:

  1. Change the index calculation logic. I will explain using 1-D for simplicity.

    Previously we have (in pseudo code):

    // 1. get the float locations from grid
    scalar_t x = from_grid()
    
    // 2. find the integral surrounding indices
    int x_left = floor(x)
    int x_right = x_left + 1
    
    // 3. calculate the linear interpolate weights
    scalar_t w_left = x_right - x
    scalar_t w_right = x - x_left
    
    // 4. manipulate the integral surrounding indices if needed
    // (e.g., clip for border padding_mode)
    x_left = manipulate(x_left, padding_mode)
    x_right = manipulate(x_right, padding_mode)
    
    // 5. interpolate
    output_val = interpolate(w_left, w_right, x_left, x_right)
    

    This is actually incorrect (and also unintuitive) because it calculates the
    weights before manipulate out-of-boundary indices. Fortunately, this
    isn't manifested in both of the current supported modes, 'zeros' and
    'border' padding:

    • 'zeros': doesn't clip
    • 'border': clips, but for out-of-bound x both x_left and x_right are
      clipped to the same value, so weights don't matter

    But this is a problem with reflection padding, since after each time we reflect,
    the values of w_left and w_right should be swapped.

    So in this commit I change the algorithm to (numbers corresponding to the
    ordering in the above pseudo-code)

    1. get float location
    4. clip the float location 
    2. find the integral surrounding indices
    3. calculate the linear interpolate weights
    

    In the backward, because of this change, I need to add new variables to track
    d manipulate_output / d manipulate_input, which is basically a multiplier
    on the gradient calculated for grid. From benchmarking this addition doesn't
    cause obvious slow downs.

  2. Implement reflection padding. The indices will keep being reflected until
    they become within boundary.

    Added variant of clip_coordinates and reflect_coordinates to be used in
    backward. E.g.,

    // clip_coordinates_set_grad works similarly to clip_coordinates except that
    // it also returns the `d output / d input` via pointer argument `grad_in`.
    // This is useful in the backward pass of grid_sampler.
    scalar_t clip_coordinates_set_grad(scalar_t in, int64_t clip_limit, scalar_t *grad_in)

    For example, if in is clipped in 'border' mode, grad_in is set to 0.
    If in is reflected odd times in 'reflection' mode, grad_in
    is set to -1.

  3. Implement nearest interpolation.

  4. Add test cases

  5. Add better input checking
    Discussed with @goldsborough for moving operator<< of at::Device,
    at::DeviceType and at::Layout into at namespace. (Otherwise
    AT_CHECK can't find them.)

  6. Support empty tensors. cc @gchanan

    • Make empty tensors not acceptable by cudnn.
    • Add AT_ASSERT(kernel block size > 0) if using GET_BLOCKS
    • Cache numel in TensorGeometry
      I was going to use numel to test if cudnn descriptor should accept a
      tensor, but it isn't used eventually. I can revert this if needed.
  7. Add more test cases, including on input checking and empty tensors

  8. Remove an obsolete comment

  9. Update docs. Manually tested by generating docs.

@ssnl ssnl mentioned this pull request Jul 31, 2018
@ssnl ssnl force-pushed the gridsample_imp branch 5 times, most recently from c43051a to 6c24a94 Compare July 31, 2018 22:43

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@ssnl ssnl force-pushed the gridsample_imp branch 10 times, most recently from 81119a9 to cfbc8cd Compare August 1, 2018 20:24
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.

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

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.

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

@ssnl ssnl force-pushed the gridsample_imp branch 4 times, most recently from cdd63c4 to f569de1 Compare August 1, 2018 21:59
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.

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

@ezyang
Copy link
Contributor

ezyang commented Aug 3, 2018

Thank you, the new comment is very clear :)

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.

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

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.

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

Copy link
Contributor

@soumith soumith left a comment

Choose a reason for hiding this comment

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

lgtm!!!

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.

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

@ssnl ssnl deleted the gridsample_imp branch August 10, 2018 19:46
zdevito pushed a commit to zdevito/ATen that referenced this pull request Aug 10, 2018
Summary:
closes #9702 .

cc jph00

Commit structure:

1. Change the index calculation logic. I will explain using 1-D for simplicity.

	Previously we have (in pseudo code):

	```
	// 1. get the float locations from grid
	scalar_t x = from_grid()

	// 2. find the integral surrounding indices
	int x_left = floor(x)
	int x_right = x_left + 1

	// 3. calculate the linear interpolate weights
	scalar_t w_left = x_right - x
	scalar_t w_right = x - x_left

	// 4. manipulate the integral surrounding indices if needed
	// (e.g., clip for border padding_mode)
	x_left = manipulate(x_left, padding_mode)
	x_right = manipulate(x_right, padding_mode)

	// 5. interpolate
	output_val = interpolate(w_left, w_right, x_left, x_right)
	```

	This is actually incorrect (and also unintuitive) because it calculates the
	weights before manipulate out-of-boundary indices. Fortunately, this
	isn't manifested in both of the current supported modes, `'zeros'` and
	`'border'` padding:

	+ `'zeros'`: doesn't clip
	+ `'border'`: clips, but for out-of-bound `x` both `x_left` and `x_right` are
	  clipped to the same value, so weights don't matter

	But this is a problem with reflection padding, since after each time we reflect,
	the values of `w_left` and `w_right` should be swapped.

	So in this commit I change the algorithm to (numbers corresponding to the
        ordering in the above pseudo-code)

	```
	1. get float location
	4. clip the float location
	2. find the integral surrounding indices
	3. calculate the linear interpolate weights
	```

	In the backward, because of this change, I need to add new variables to track
	`d manipulate_output / d manipulate_input`, which is basically a multiplier
	on the gradient calculated for `grid`. From benchmarking this addition doesn't
	cause obvious slow downs.

2. Implement reflection padding. The indices will keep being reflected until
	they become within boundary.

	Added variant of `clip_coordinates` and `reflect_coordinates` to be used in
	backward. E.g.,
	```cpp
	// clip_coordinates_set_grad works similarly to clip_coordinates except that
	// it also returns the `d output / d input` via pointer argument `grad_in`.
	// This is useful in the backward pass of grid_sampler.
	scalar_t clip_coordinates_set_grad(scalar_t in, int64_t clip_limit, scalar_t *grad_in)
	```
	For example, if `in` is clipped in `'border'` mode, `grad_in` is set to `0`.
	If `in` is reflected **odd** times in `'reflection'` mode, `grad_in`
	is set to `-1`.

3. Implement nearest interpolation.

4. Add test cases

5. Add better input checking
  Discussed with goldsborough for moving `operator<<` of `at::Device`,
  `at::DeviceType` and `at::Layout` into `at` namespace. (Otherwise
  `AT_CHECK` can't find them.)

6. Support empty tensors. cc gchanan

    + Make empty tensors not acceptable by cudnn.
    + Add `AT_ASSERT(kernel block size  > 0)` if using `GET_BLOCKS`
   + Cache `numel` in `TensorGeometry`
      I was going to use `numel` to test if cudnn descriptor should accept a
      tensor, but it isn't used eventually. I can revert this if needed.

7. Add more test cases, including on input checking and empty tensors

8. Remove an obsolete comment

9. Update docs. Manually tested by generating docs.
Pull Request resolved: pytorch/pytorch#10051

Differential Revision: D9123950

Pulled By: SsnL

fbshipit-source-id: ac3b4a0a36b39b5d02e83666cc6730111ce216f6
ezyang added a commit that referenced this pull request Aug 13, 2018
goodlux pushed a commit to goodlux/pytorch that referenced this pull request Aug 15, 2018
Summary:
closes pytorch#9702 .

cc jph00

Commit structure:

1. Change the index calculation logic. I will explain using 1-D for simplicity.

	Previously we have (in pseudo code):

	```
	// 1. get the float locations from grid
	scalar_t x = from_grid()

	// 2. find the integral surrounding indices
	int x_left = floor(x)
	int x_right = x_left + 1

	// 3. calculate the linear interpolate weights
	scalar_t w_left = x_right - x
	scalar_t w_right = x - x_left

	// 4. manipulate the integral surrounding indices if needed
	// (e.g., clip for border padding_mode)
	x_left = manipulate(x_left, padding_mode)
	x_right = manipulate(x_right, padding_mode)

	// 5. interpolate
	output_val = interpolate(w_left, w_right, x_left, x_right)
	```

	This is actually incorrect (and also unintuitive) because it calculates the
	weights before manipulate out-of-boundary indices. Fortunately, this
	isn't manifested in both of the current supported modes, `'zeros'` and
	`'border'` padding:

	+ `'zeros'`: doesn't clip
	+ `'border'`: clips, but for out-of-bound `x` both `x_left` and `x_right` are
	  clipped to the same value, so weights don't matter

	But this is a problem with reflection padding, since after each time we reflect,
	the values of `w_left` and `w_right` should be swapped.

	So in this commit I change the algorithm to (numbers corresponding to the
        ordering in the above pseudo-code)

	```
	1. get float location
	4. clip the float location
	2. find the integral surrounding indices
	3. calculate the linear interpolate weights
	```

	In the backward, because of this change, I need to add new variables to track
	`d manipulate_output / d manipulate_input`, which is basically a multiplier
	on the gradient calculated for `grid`. From benchmarking this addition doesn't
	cause obvious slow downs.

2. Implement reflection padding. The indices will keep being reflected until
	they become within boundary.

	Added variant of `clip_coordinates` and `reflect_coordinates` to be used in
	backward. E.g.,
	```cpp
	// clip_coordinates_set_grad works similarly to clip_coordinates except that
	// it also returns the `d output / d input` via pointer argument `grad_in`.
	// This is useful in the backward pass of grid_sampler.
	scalar_t clip_coordinates_set_grad(scalar_t in, int64_t clip_limit, scalar_t *grad_in)
	```
	For example, if `in` is clipped in `'border'` mode, `grad_in` is set to `0`.
	If `in` is reflected **odd** times in `'reflection'` mode, `grad_in`
	is set to `-1`.

3. Implement nearest interpolation.

4. Add test cases

5. Add better input checking
  Discussed with goldsborough for moving `operator<<` of `at::Device`,
  `at::DeviceType` and `at::Layout` into `at` namespace. (Otherwise
  `AT_CHECK` can't find them.)

6. Support empty tensors. cc gchanan

    + Make empty tensors not acceptable by cudnn.
    + Add `AT_ASSERT(kernel block size  > 0)` if using `GET_BLOCKS`
   + Cache `numel` in `TensorGeometry`
      I was going to use `numel` to test if cudnn descriptor should accept a
      tensor, but it isn't used eventually. I can revert this if needed.

7. Add more test cases, including on input checking and empty tensors

8. Remove an obsolete comment

9. Update docs. Manually tested by generating docs.
Pull Request resolved: pytorch#10051

Differential Revision: D9123950

Pulled By: SsnL

fbshipit-source-id: ac3b4a0a36b39b5d02e83666cc6730111ce216f6
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.

Improve grid_sample

4 participants