Fix types of SetUp, Forward, Backward, and gradient checker calls

[longjon]

Currently, vectors of blobs that are output arguments (such as Forward's top and Backward's bottom) have the type vector<Blob*>*. This type was chosen following the rule that outputs should be always be pointers. However, the semantics of this type are not correct for its use in Caffe; they allow modification of the vector itself (such as adding blobs, or changing the elements to point to other blobs), which will break Caffe. The real output arguments in these cases are the blobs themselves, which are correctly passed in via pointers. This incongruity also leads to a bunch of line noise, since every reference to an argument of this form must be dereferenced before it can be indexed, which in turn causes confusion.

This PR changes all such output arguments to have type const vector<Blob*>&, thus cleaning up code and turning dynamic errors into static ones.

(One might also observe that input vectors don't have quite the right type -- they are also const vector<Blob*>&, which allows modification of the input blobs. Unfortunately this is more difficult to solve, as blob vectors have to be used both as inputs and outputs, and C++ lacks container covariance.)

I think there is a general consensus(?) among BVLC folk that this is the right thing to do. Nevertheless, some reasons not to merge this PR are:

• it changes a lot of types (albeit internal ones), which might interfere with people's development
• it makes the types of input and output arguments the same, which is a bit confusing (however, it's usually clear from reading whether a function goes from top to bottom or vice versa)

[bhack]

How hard is the refactor to insert input blob pointer also to the output vector when we need to write into the input blob? Opencv for example let to use the same Matrix for input and output when you want to do "in-place" operation.

[jeffdonahue]

I'm in favor of this.

(One might also observe that input vectors don't have quite the right type -- they are also const vector<Blob*>&, which allows modification of the input blobs. Unfortunately this is more difficult to solve, as blob vectors have to be used both as inputs and outputs, and C++ lacks container covariance.)

I guess Net would have to create two sets of bottom_vecs and top_vecs (an extra set of bottom_vecs with const blob pointers for the forward pass, and an extra set of top_vecs with const blob pointers for the backward pass) for this to work? That wouldn't be too bad (just a few extra lines of basically duplicate code), I'd think. But maybe not worth it anyway. But if we're ever going to consider doing something like that we should probably do it right now with this PR, so I thought it would be good to bring up...

[longjon]

@bhack, I don't know what you are suggesting that is different from what already exists; in-place operations are supported, in which case you will have top == bottom.

@jeffdonahue, nice suggestion, I'll look into it. There are some complications, such as the implementation of compositions (e.g., in LRN layer) which will need the same redundancy as Net (however, compositions are already pretty redundant, and should probably be done with a different mechanism anyway). Also, hinge loss layer begins the backward computation in the forward pass, using bottom diff; that should probably just be fixed to not happen.

[bhack]

@longjon nevermind, was similar to what @jeffdonahue proposed. The non const pointers in the net passed to forward/back is the "output vector"

[Yangqing]

I am in general not a big fan of breaking coding convention (for example, the const may give people an impression that this should not be changed). But in this case there are both pros and cons, and the problem is mainly because of C++'s flaky definition of const decorator, so looks good to me :)

[kloudkl]

If the input and the output types must have different forms, the original types can be replaced by explicit aliases typedef const vector<Blob*>& InputBlobVector and typedef const vector<Blob*>& OutputBlobVector which is done in OpenCV.

[bhack]

In the case of inputblob also Blob* need to be const?

[longjon]

I looked into @jeffdonahue's suggestion of using const vector<const Blob*>& for input vectors, requiring (as noted above) a separate vector to be constructed whenever a vector<Blob*> needs to be used as an input vector. I found this to be a bit more cumbersome than obviously warranted; additional code (and a modicum of additional storage) are required in more places than Net, and adding the const is a commitment to constructing such circumlocutions when needed in the future. (Nevertheless, the changes did expose some const errors, which I'll attempt to correct in this PR.)

Furthermore, the const correctness itself doesn't change the content of the layer code, unlike the agreed upon pointer dereference, so it seems reasonable to me to push the latter through while keeping the former in mind in the future.

I'll note that that there are (at least) two other possible ways to inject constness for input blobs:

1. Pass const_iterator instead of vector. (Note that this still allows random access to blobs.) Pros: gets the right types with no extra storage or additional lines of code. Easy to switch to something that is not a std::vector in the future. Cons: One argument becomes two, because both begin and end need to be passed. For uniformity, output vectors should be changed as well, so two arguments become four. Code that reads the length of the vector needs to change.
2. Pass some custom type that wraps a vector and provides random access to const Blob*s and a size method. Pros: same interface, const correctness. Cons: types are no longer just taken from std, so that everyone can recognize them.

An orthogonal issue mentioned above is whether we should use typedefs instead of explicitly specified types. Usually typedefs are a good thing, and will allow types to be changed in the future more easily. The disadvantage is that someone new to the codebase now has to stop and think "InputBlobVector? What's that?" instead of seeing std::vector and immediately grokking the layer interface.

Here is my suggested course of action for this PR:

1. Block until On-the-fly net resizing, without reallocation (where possible) #594 has converged, as this merge conflicts heavily with that one, and that one is more up-to-date.
2. I'll rebase this, just removing the extra pointers from output vectors as before. This is the only possibly anticipated change which should affect the bodies of layer functions (with some minor exceptions, like HingeLossLayer).
3. Merge this minimal breaking change.
4. Consider improving const correctness and/or readability in the future, without having to change layer code, by 1 or 2 above, or using typedefs. (Or, reject these options as overengineering.)

[jeffdonahue]

suggested course of action SGTM

[longjon]

For those who want to update their own branches or layers according to the changes made here, this is the complete list of sed commands that were used to update dev.

s/(\*bottom)/bottom/g
s/(\*top)/top/g
s/ReshapeLiketop/ReshapeLike(*top)/g
s/top->size/top.size/g
s/bottom->size/bottom.size/g
s/vector<Blob<Dtype>\*>\* bottom/const vector<Blob<Dtype>*>\& bottom/
s/vector<Blob<Dtype>\*>\* top/const vector<Blob<Dtype>*>\& top/
s/&top_vecs/top_vecs/g
s/&bottom_vecs/bottom_vecs/g
s/\(layer_\?->SetUp(\w\+, \)&\(\w\+)\)/\1\2/
s/\(layer_\?->Reshape(\w\+, \)&\(\w\+)\)/\1\2/
s/\(layer_\?->Forward(\w\+, \)&\(\w\+)\)/\1\2/
s/\(layer_\?->Backward(\w\+, \w\+, \)&\(\w\+)\)/\1\2/
s/CheckBlobCounts(bottom, \*top)/CheckBlobCounts(bottom, top)/
s/&square_bottom_vec_/square_bottom_vec_/
s/&(\(this->blob_bottom_vec_.\?\))/\1/g
s/&\(\(this->\)\?blob_bottom_vec_.\?\)/\1/g
s/&(\(\(this->\)\?\(sep_\)\?blob_top_vec\w*\))/\1/g
s/&\(\(this->\)\?blob_top_vec_\)/\1/g
s/layer->SetUp(\*bottom/layer->SetUp(bottom/
s/layer->Reshape(\*bottom/layer->Reshape(bottom/
s/layer->Forward(\*bottom/layer->Forward(bottom/
s/layer->Backward(\*top/layer->Backward(top/

[sguada]

Wow @longjon this just broke my PR #1070 I will rebase it again, but hope it gets reviewed @jeffdonahue soon.