Closed thelukester92 closed 7 years ago
The deltas should probably also be mutable (i.e. const backprop); only applying the deltas actually changes the network, so only that method should not be const.
Without looking at the code, I believe this is for speed so that we don't have to recalculate some terms. It definitely should be const from a theory point of view. Perhaps we should have two forwardProp methods, one for training ( ie the "optimized" version ) and one for prediction.
We wouldn't need to recalculate if we changed the variables to mutable. That would allow a const method to change them still. A separate function might work, as well.
Ah, I see. That sounds like the perfect solution then, including the deltas.
Thought of a reason to cancel this: recurrent networks do have an internal state that are expected to change in the forward prop step.
Closing; we might someday revisit this, but I see no need for it at this point.
I think it is reasonable to assume that "const" function are also reentrant. (That is, you can call them from multiple threads at the same time, and expect them to work. People who build ensembles are particularly interested in having reentrant predict methods because they usually want to parallelize their ensembles.) Using the "mutable" keyword breaks that assumption by enabling the method to be const without being reentrant. To me, that seems more deceptive than helpful. What is the value of being const without being reentrant?
It seems like forwardProp should be const, but it currently is not because it modifies the state of the network (by changing net inputs to neurons, activations, etc). One possible way to fix this is to make those "temporary" variables (like net and activation) mutable, so that they are changeable even in const methods. I believe this should be the expected behavior.
Any reason why this would be a bad move?