There are two ways so called array of structures (AoS) and structure of arrays (SoA) to store multiple complex objects. vector<bool> propagate_down, vector<Blob<Dtype>*>* bottom is an example of SoA. AoS is sometimes simpler, since tightly related properties belong to the same object and are always passed around together. If propagate_down becomes a property of Blob, the two vectors can be boiled down to a single vector<Blob<Dtype>*>* bottom.

True, one could store a bool in the blobs themselves to indicate whether they need their diffs computed. I don't have a very strong preference. I'd guess this interface makes it less likely one will just ignore the propagate_down in their Backward implementation, and less likely one will forget to correctly set propagate_down before calling the function. Open to arguments for changing though.

Kudos for implementing a long-desired feature :)

I would vote for the current approach. Propagate_down, as a property of a
Blob, may be highly confusing (note that Blob is just a multi-dimensional
array - what does it mean by saying "an array needs gradients propagated
down"?) It is only when we put things in the context of a neural network
when we need this variable. Also, the propagate down value is computed by
the network not by the blob, so it makes more sense that the network
bookkeeps them, not the blob.

Yangqing

On Thu, Jun 12, 2014 at 6:44 PM, Jeff Donahue notifications@github.com
wrote:

True, one could store a bool in the blobs themselves to indicate whether
they need their diffs computed. I don't have a very strong preference. I'd
guess this interface makes it less likely one will just ignore the
propagate_down in their Backward implementation, and less likely one will
forget to correctly set propagate_down before calling the function. Open to
arguments for changing though.

Reply to this email directly or view it on GitHub
#497 (comment).

Thanks for the input @Yangqing! I'll leave the interface as is then.

Don't merge this yet; I realized there is a bit more work to do -- this (probably, haven't tested) breaks setting force_backward: true (which I never use so I didn't think about it) as that would cause propagate_down to be set to true for label inputs (e.g. to SoftmaxLossLayer), and with this PR Caffe will crash if requested to propagate_down any inputs it doesn't know how to propagate down.

@jeffdonahue agreed on the new interface. I could test force_backward once you make your fix with the gradient map code.

p.s. Although this PR shows the two initial commits shared with #495, they are already merged and will be gracefully dropped when this PR is merged (for the curious...).

Just rebased as such. Thanks for merging the refactoring!

I don't know if force_backward is actually needed, specially since there are layers and blobs for which doesn't make sense to compute backward, or even cannot.
I think it was only used to compute gradients back to the image. However in the new MatCaffe #501 I included an example of computing the gradients and didn't need to use it.

Why the NOT_IMPLEMENTED; // Cannot backprop to targets. in loss layers? I'd prefer a LOG(FATAL) << "Cannot back-propagate to targets."; or the like.

I read the "yet" as meaning backpropagation to targets could have some sense and might be implemented, but except for distances like Euclidean loss (and perhaps sigmoid cross entropy loss) it isn't right. No?

Thanks for taking a look at this Evan! I changed to a more appropriate LOG(FATAL) as suggested, and fixed the force_backward bug by adding another layer property AllowForceBackward(const int bottom_index) { return true; } which loss layers override with bottom_index != 1 to disallow backprop to labels.

as far as I can recall, force_backward had an earlier history in decaf where the propagate_down functionality was actually implemented, and as a result if I wanted to get the gradients w.r.t. the images I had to force_backward so that conv1 computes the gradients (otherwise, since data layer never requires propagate down, conv1 will only compute gradients w.r.t. the filters and not the input).

This is not that useful for now I guess, and I agree that removing this (kind of flaky) functionality is better if supporting it is too much of a hassle.

I'm going to rebase and merge to move on to weight sharing.

Re: the necessity of force_backward, we can drop it if we like in a future PR (since potentially prototxts will have to change). Sergio can test with matcaffe and I'll test pycaffe once all the related PRs are in.