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

ND Crop layer #3570

Merged
merged 4 commits into from
Mar 5, 2016
Merged

ND Crop layer #3570

merged 4 commits into from
Mar 5, 2016

Conversation

BlGene
Copy link
Contributor

@BlGene BlGene commented Jan 19, 2016

Something along the lines of what a ND crop layer would look like.
Based on: PR #1976 -> PR #3565 -> this

Todo

  • Test for D > 4
  • Test backwards
  • GPU kernel copy instead of memcopy (?)
  • const indices and offsets
  • GPU backwards/forwards
  • CPU backwards
  • parameter size checks
  • lint

Please see PR #3613 for how to set the crop parameters.

@BlGene
Copy link
Contributor Author

BlGene commented Jan 20, 2016

@longjon So, my attempt at a ND crop layer, if this is approximately what you are looking for I can try to fix the GPU part.

Edit: For the GPU part just implementing the 4D case would seem like a reasonable compromise.

@longjon
Copy link
Contributor

longjon commented Jan 21, 2016

Actually I'm confused about the parameter interface here... what I had in mind is something mirroring the conv layer interface, with a scalar (i.e., optional) param axis alongside a repeated param of crop values (as you have currently).

The way it would work would be the same way conv layer works: if only one crop value is specified, all the dimensions after the specified axis would be cropped (by the same amount); otherwise the number of crop values specified must be equal to the number of dimensions following axis, and the trailing dimensions would be cropped accordingly.

The reason for doing it this way is because the crop values need to be determined from the net specification, and the net specification doesn't know how many dimensions the data will have! If the layers we are "cropping through" include conv/pool layers with non-singleton kernel sizes, then we know exactly how many dimensions to crop; but if not, all we can do is propagate the axis param and determine how many dimensions to crop at run time. (At least, I think that should work.)

It would be nice to have an ND GPU kernel, but it's fine to punt that to get the basic functionality in.

Thanks!

@BlGene
Copy link
Contributor Author

BlGene commented Jan 21, 2016

@longjon The parameters should do what you want now.

I substituted the copy kernel with repeated calls to caffe_copy, which if I understood correctly will call cudaMemcpy, and thus perform the copy operation on the GPU. My intuition is that calling this thousands of times is probably less efficient than writing a custom kernel.

@longjon
Copy link
Contributor

longjon commented Jan 22, 2016

The parameters look right now.

caffe_copy will perform the copy on the GPU, but (as you suggest) has overhead -- even in the 2D case I found the overhead unacceptable, which is why the kernel exists. A fine solution for now would be to keep the existing kernel around, and use it only for the innermost two dimensions, looping around the outer ones. That should keep the performance the same in the 2D case while supporting ND.

@BlGene BlGene force-pushed the crop-nd branch 2 times, most recently from bcbd2f7 to c092887 Compare January 26, 2016 15:44
@BlGene
Copy link
Contributor Author

BlGene commented Jan 26, 2016

@longjon The crop_copy_gpu function now calls the kernel for the innermost two dimensions. This should hopefully fix the speed problem.

@BlGene BlGene changed the title ND Crop layer [WIP] ND Crop layer Feb 1, 2016
@bjosv79
Copy link

bjosv79 commented Feb 19, 2016

It seems to a few problems with the backward pass of crop_layer.cu:

  • A typo src_outer_stride instead of src_inner_stride in line 94.
  • Incorrect number of elements in caffe_gpu_set in line 89.
  • Is the caffe_gpu_set really necessary? It is already done in line 119?

@BlGene BlGene force-pushed the crop-nd branch 2 times, most recently from 8495199 to 164472e Compare February 19, 2016 12:12
@BlGene
Copy link
Contributor Author

BlGene commented Feb 19, 2016

@bjosv79 Thanks. All those things seemed correct, I changed them ( but haven't tested yet ).

@@ -306,7 +306,7 @@ message ParamSpec {
// NOTE
// Update the next available ID when you add a new LayerParameter field.
//
// LayerParameter next available layer-specific ID: 143 (last added: scale_param)
// LayerParameter next available layer-specific ID: 144 (last added: crop_param)
Copy link
Member

Choose a reason for hiding this comment

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

Needs ID++ after #3211 -- sorry for the rebase.

@shelhamer
Copy link
Member

@BlGene I pushed a rebase on the latest master to my fork https://github.com/shelhamer/caffe/tree/crop-nd with a few other additions -- feel free to integrate my changes if the commits save you time.

// values specified must be equal to the number of dimensions following axis, and
// the trailing dimensions would be cropped accordingly.
// Protip: standard dimensions are ( N,C,H,W )
optional uint32 axis = 1 [default = 2];
Copy link
Member

Choose a reason for hiding this comment

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

Let's make axis mirror the axis arg of other layers to allow for negative indexing through an int32 field and the CanonicalAxisIndex() helper method? See for instance https://github.com/BVLC/caffe/blob/master/src/caffe/layers/flatten_layer.cpp#L12-L13

shelhamer added a commit to longjon/caffe that referenced this pull request Feb 28, 2016
shelhamer added a commit to shelhamer/caffe that referenced this pull request Mar 4, 2016
Python/net spec coordinate map and crop computation

* longjon/py-coord-map:
  [pycaffe] test coord_map
  [pycaffe] align coord_map and BVLC#3570 Crop layer
  [pycaffe] document, style, and complete coord_map
  [pycaffe] add coord_map.py for computing induced coordinate transform
shelhamer added a commit to longjon/caffe that referenced this pull request Mar 5, 2016
- crop -> offset
- adjust crop axis by 1
@BlGene BlGene force-pushed the crop-nd branch 2 times, most recently from 66216cf to e03a287 Compare March 5, 2016 14:22
@BlGene
Copy link
Contributor Author

BlGene commented Mar 5, 2016

@shelhamer OK, I fixed the commit messages.

I'm happy to have FCN functionality merged too. Thanks for the attention to detail. 👍

longjon and others added 4 commits March 5, 2016 14:59
configure offset(s) through proto definition.
Changes are:
  reduce test blob dims for speed
  use standard Gaussian filler,
  polish formatting and rename tests,
  test HW crop and 5D crop,
  standard gradient checks
shelhamer added a commit that referenced this pull request Mar 5, 2016
@shelhamer shelhamer merged commit ca1ce4a into BVLC:master Mar 5, 2016
@BlGene BlGene deleted the crop-nd branch March 6, 2016 16:21
zouxiaochuan pushed a commit to zouxiaochuan/caffe that referenced this pull request Mar 17, 2016
- crop -> offset
- adjust crop axis by 1
@elezar
Copy link
Contributor

elezar commented Mar 31, 2016

Just a quick question from my side, can the ND crop be used to implement a rectangular crop as in #1980?

@elezar elezar mentioned this pull request Mar 31, 2016
@BlGene
Copy link
Contributor Author

BlGene commented Mar 31, 2016

This layer crops blob B to have the same shape as Blob A. So its a bit hacky, but you could create a dummy blob (A) that has the desired shape and crop the (B) blob to this size.

BR, Max

SvenTwo pushed a commit to SvenTwo/caffe that referenced this pull request Apr 6, 2016
- crop -> offset
- adjust crop axis by 1
emaggiori pushed a commit to emaggiori/caffe that referenced this pull request Apr 8, 2016
- crop -> offset
- adjust crop axis by 1
sato-cloudian pushed a commit to sato-cloudian/caffe that referenced this pull request Aug 10, 2016
- crop -> offset
- adjust crop axis by 1
fxbit pushed a commit to Yodigram/caffe that referenced this pull request Sep 1, 2016
- crop -> offset
- adjust crop axis by 1
fxbit pushed a commit to Yodigram/caffe that referenced this pull request Sep 1, 2016
zouxiaochuan pushed a commit to zouxiaochuan/caffe that referenced this pull request Oct 24, 2016
- crop -> offset
- adjust crop axis by 1
zouxiaochuan pushed a commit to zouxiaochuan/caffe that referenced this pull request Oct 24, 2016
- crop -> offset
- adjust crop axis by 1
zouxiaochuan pushed a commit to zouxiaochuan/caffe that referenced this pull request Feb 15, 2017
- crop -> offset
- adjust crop axis by 1
zouxiaochuan pushed a commit to zouxiaochuan/caffe that referenced this pull request Feb 15, 2017
- crop -> offset
- adjust crop axis by 1
zouxiaochuan pushed a commit to zouxiaochuan/caffe that referenced this pull request Feb 15, 2017
- crop -> offset
- adjust crop axis by 1
zouxiaochuan pushed a commit to zouxiaochuan/caffe that referenced this pull request Feb 15, 2017
- crop -> offset
- adjust crop axis by 1
acmiyaguchi pushed a commit to acmiyaguchi/caffe that referenced this pull request Nov 13, 2017
- crop -> offset
- adjust crop axis by 1
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.

5 participants