Thanks @kmatzen, we need to be careful about this. Does this PR exhaust the cases where this mistake was made?

Just glancing at ReLU, it looks like the error is in the test, due to cavalier copy and paste. Perhaps having the gradient checker print layer parameters would have kept this from going unnoticed?

"Does this PR exhaust the cases where this mistake was made?"

I don't know. I just greped through the tests directory to find ParseFromString.

"Perhaps having the gradient checker print layer parameters would have kept this from going unnoticed?"

I would just CHECK everything that could fail, especially any Google API since they rely on error codes rather than exceptions to indicate failure. CHECK(foo.ParseFromString(bar)); would have just as easily indicated a problem with the test case.

Yep, the grep is all I mean.

Of course the calls should be checked. This is a case where one error masked another, causing tests to pass without testing anything, which is worse than not having the test in the first place. So now I am wondering, how might we tweak the testing framework to make mistakes of this type harder to make in the future?

Not sure how we'd automate it but for setting one or two parameters we should probably discourage the use of TextFormat. e.g. instead of:

CHECK(google::protobuf::TextFormat::ParseFromString(
    "mvn_param{normalize_variance: false}", &layer_param));

instead write layer_param.mutable_mvn_param()->set_normalize_variance(false). Then as long as it compiles, you know you won't have a runtime problem (either silent failure or check failure).

FWIW, that's what I would do.

I am OK with using text format since it makes testing code easier to read, but also like the explicit setting.

Maybe we should set "-Wunused-result" so that if a function returns something and the code is not using it, we'll be able to catch it.

This bug seems to have two parts: (1) the unit tests are wrong and (2) fixing the unit tests reveals underlying bugs in some of the layers.

Some of the this is addressed by #1979: the MVNLayer backward pass was incorrect, and the ParseFromString bug prevented the incorrect backward code from being discovered.

In addition to what seanbell said, there was also a problem with MVNLayer::Forward with across_channels = true. That was also not being tested (due to the ParseFromString problem). When it was tested, it would produce NaNs. The problem there was that sum_multiplier_ member was the wrong size for the case across_channels = true. If I remember all this correctly. That should also be fixed in 1979.

Closing as the last instance of this issue is noted and addressed by #1979.