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

Blobs are N-D arrays (for N not necessarily equals 4) #1970

Merged
merged 35 commits into from
Mar 4, 2015

Conversation

jeffdonahue
Copy link
Contributor

Replaces #1486 -- this PR is to master instead of dev. This is rebased and is ready for review @longjon.

This PR gives Blobs a vector<int> shape_ of dimensions, rather than the old num, channels, height, width. The first commit is all the changes needed to get Caffe to compile and everything to run as before with the new vector of tensor dimensions. The remaining commits generalize some existing classes to use the new tensor dimensions (but they are not necessary to make it run, as it's still fine to just use all 4-D blobs with extra singleton dimensions where needed).

@jeffdonahue
Copy link
Contributor Author

I had added a commit intended to add matcaffe support for N-D array blobs, but decided to defer to #1913 to add the support, as I don't have much experience with matcaffe and it might require some special cases (e.g., I remembered that MATLAB doesn't support arrays with <2 axes so would need to figure out what to do with those cases -- maybe the MATLAB default of adding extra axes with dimension 1 would work fine, but not sure). But if that commit might be helpful in the development of #1913, feel free to cherry-pick or just refer to it here (but note that I didn't test or even try to compile it...).

So for now, matcaffe will continue to work fine for blobs with <= 4 axes, but will die on attempts to access blobs with >4 dimensions (per their use of num/channels/height/width, which call LegacyShape).

@@ -2,13 +2,21 @@ syntax = "proto2";

package caffe;

// Specifies the shape (dimensions) of a Blob.
message BlobShape {
repeated int64 dim = 1 [packed = true];
Copy link
Member

Choose a reason for hiding this comment

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

My 2 cents - uint32 should be enough? A 2G blob (8G in floats) looks big enough.

(But yeah, 640k used to be enough for everything.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My feeling is that it's probably worth future-proofing given the relatively tiny amount of extra storage cost (unless you're really using blobs with INT_MAX axes, as are now allowed...). I could make it uint32 for now and then later change it to int64 if/when we really find practical uses for blobs that large, but that might cause backward compatibility problems with (de)serialization that I don't even want to think about... (Why not uint64? I think it's worth keeping -1 and other <0 values reserved for various purposes -- e.g. setting axis [default = -1] to signify "all axes", or that an axis must be manually specified and should fail otherwise, etc. And it does seem pretty unlikely that in the reasonably foreseeable future we'll need 2^64 byte blobs, so 2^63-1 bytes seems like an okay max...)

I'm open to discussion on any of this though.

Copy link
Member

Choose a reason for hiding this comment

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

Future-proof sounds better :) Thanks for checking with me!

Copy link
Member

Choose a reason for hiding this comment

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

But Jeff, there is no reason for any individual to have a > 2GB blob in their home. Haha no, I'm all for int64 dimensions.

@longjon
Copy link
Contributor

longjon commented Mar 2, 2015

Oops, just realized that I've been commenting on commits again. Agree with int64 blob dimensions. I'm ready to merge this today, pending comments made, the new type for Python Blob.reshape (which I'll do), and a nod from @shelhamer.

@jeffdonahue jeffdonahue force-pushed the tensor-blob branch 2 times, most recently from 221b2f0 to 7419b79 Compare March 2, 2015 23:36
@longjon
Copy link
Contributor

longjon commented Mar 2, 2015

Added my Python update... this should be ready pending your approval of that, Travis, and @shelhamer.

@jeffdonahue
Copy link
Contributor Author

Thanks Jon! Your updated Blob_Reshape looks good to me.

@shelhamer
Copy link
Member

The axis field in the proto is inconsistently (u)int32 -- should it be int32 everywhere to allow positive and negative indexing in Slice, Concat, and InnerProduct dimension picking? Or it's fine to keep this the same for now as current behavior.

K_ = bottom[0]->count() / bottom[0]->num();
const int dim = this->layer_param_.inner_product_param().axis();
// Dimensions starting from "axis" are "flattened" into a single
// length K_ vector. For example, if bottom[0]'s shape is (N, C, H, W),
Copy link
Member

Choose a reason for hiding this comment

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

For example, if bottom[0]'s shape is (N, C, H, W) and axis = 1

@shelhamer
Copy link
Member

@jeffdonahue this is sweet. My only comments were minor, so do what you want with them and merge!

@@ -35,6 +35,9 @@ void SoftmaxWithLossLayer<Dtype>::Reshape(
const vector<Blob<Dtype>*>& bottom, const vector<Blob<Dtype>*>& top) {
LossLayer<Dtype>::Reshape(bottom, top);
softmax_layer_->Reshape(softmax_bottom_vec_, softmax_top_vec_);
softmax_axis_ = this->layer_param_.softmax_param().axis();
outer_num_ = bottom[0]->count(0, softmax_axis_);
inner_num_ = bottom[0]->count(softmax_axis_ + 1);
Copy link
Member

Choose a reason for hiding this comment

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

This could be a good time to add a check that the prediction and target dimensions (except channel) agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in a717963 -- I was a little less strict about it, just checking that the number of labels matches the number of predictions, as I couldn't figure out a way to do it that's backward compatible and nice to use in the future. (For example, I didn't want to break the current behavior of (NxCxHxW) predictions with (Nx1xHxW) labels, but it seems silly to require the singleton axis in the labels, but requiring (NxHxW) labels -- no singleton axis -- breaks backwards compatibility.)

@jeffdonahue
Copy link
Contributor Author

Hey Evan, thanks for the review! I made the additional commits above in response to your comments -- currently marked with fixup! to be autosquashed later. I thought you might want to take a look since, re negative indexing, I applied it in SliceLayer, ConcatLayer, SoftmaxLayer, and InnerProductLayer, and ended up adding an additional Blob method CanonicalAxisIndex (as I started to realize I was writing the same checks several times). I added a single test that actually uses it to TestConcatLayer.

Edit: I went ahead and squashed the fixups, but I saved the state before the squash in another branch "tensor-blob-presquash" if you still want to look at only the additional changes I made after your review.

@jeffdonahue jeffdonahue force-pushed the tensor-blob branch 3 times, most recently from 1ef0e32 to 8a83d07 Compare March 3, 2015 08:03
jeffdonahue added a commit to jeffdonahue/caffe that referenced this pull request Mar 9, 2015
jeffdonahue added a commit to jeffdonahue/caffe that referenced this pull request Mar 9, 2015
jeffdonahue added a commit to jeffdonahue/caffe that referenced this pull request Mar 9, 2015
jeffdonahue added a commit that referenced this pull request Mar 9, 2015
Fixup AccuracyLayer like SoftmaxLossLayer in #1970
qinhongwei pushed a commit to qinhongwei/caffe that referenced this pull request Mar 12, 2015
jeffdonahue added a commit to jeffdonahue/caffe that referenced this pull request Mar 26, 2015
jeffdonahue added a commit to jeffdonahue/caffe that referenced this pull request Mar 26, 2015
jeffdonahue added a commit to jeffdonahue/caffe that referenced this pull request Mar 26, 2015
jeffdonahue added a commit to jeffdonahue/caffe that referenced this pull request May 15, 2015
shelhamer added a commit that referenced this pull request May 15, 2015
Update docs for ND blobs (#1970) and layer type is a string (#1694)
myfavouritekk added a commit to myfavouritekk/caffe that referenced this pull request May 15, 2015
* master: (21 commits)
  Update docs for ND blobs (BVLC#1970) and layer type is a string (BVLC#1694)
  Add ReshapeParameter axis and num_axes to reshape only a particular span of the input shape
  basic tests (Forward, Gradient) for ReshapeLayer
  ReshapeLayer fixups for ND blobs
  Added a Reshape layer for copying-free modification of blob dimensions.
  Spatial Pyramid Pooling Layer
  remove bogus implementation of SigmoidCrossEntropyLossLayer::Forward_gpu
  remove superfluous empty destructors
  [pycaffe] use bp::object instead of PyObject* for self in Python layer
  python: PEP8; changed docstring documentation style to NumPyDoc style
  This imports the wrong io module in Python 3.
  check that count_ does not overflow in Blob::Reshape
  Modify for better readability regarding temporary bufffer for backward computation
  Fix redundancy of parameter backward computation
  Added support for original implementation, using (margin - d^2), through the legacy_version parameter.
  added epsilon to prevent possible division by zero in gradient calculation
  Fixed contrastive loss layer to be the same as proposed in Hadsell et al 2006
  remove spurious net.hpp includes
  always call Layer::Reshape in Layer::Forward
  Increment iter_ before snapshotting, remove +1 logic -- fixes final snapshot being off by one
  ...
matthiasplappert pushed a commit to matthiasplappert/caffe that referenced this pull request Aug 10, 2015
cbfinn pushed a commit to cbfinn/caffe that referenced this pull request Aug 12, 2015
cbfinn pushed a commit to cbfinn/caffe that referenced this pull request Aug 12, 2015
cbfinn pushed a commit to cbfinn/caffe that referenced this pull request Aug 12, 2015
wangyida pushed a commit to wangyida/caffe that referenced this pull request Sep 22, 2015
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.

9 participants