Skip to content

Bugfix for #2017 broken across_channels computation in MVNLayer#2958

Closed
cdoersch wants to merge 1 commit intoBVLC:masterfrom
cdoersch:mvnlayer_acrosschannels
Closed

Bugfix for #2017 broken across_channels computation in MVNLayer#2958
cdoersch wants to merge 1 commit intoBVLC:masterfrom
cdoersch:mvnlayer_acrosschannels

Conversation

@cdoersch
Copy link
Contributor

The current implementation of MVNLayer requires multiplying the data matrix by a vector of 1's to perform sums for the mean and variance. This vector, sum_multiplier, gets created with the incorrect size when across_channels is true (it needs to be channels*width*height). See #2017. This PR checks for the across_channels flag and creates the sum_multiplier with the correct size.

Note that this is fixed in #1979 but it's looking less and less likely that this PR will be merged.

@seanbell can you confirm that this fixes all the bugs you're aware of in mvnlayer?

@longjon longjon added the bug label Aug 23, 2015
@longjon
Copy link
Contributor

longjon commented Aug 23, 2015

Thanks for breaking out this fix, @cdoersch, and to answer @jyegerlehner and @seanbell from #1979, that's the right thing to do in this situation.

@seanbell claimed this was not failing a test because the test was also incorrect; is that still true? Ideally this PR (and most(?) bug fix PRs) should have two commits: one that fixes the test and fails Travis, and one that corrects the bug and passes. That way we have good confidence that the fix won't regress in the future!

@jyegerlehner
Copy link
Contributor

@cdoersch my recollection is there are a couple more. 1) the backward gradient calculation was wrong in the mean-only case. 2) The tests weren't testing what they purport to, because they were using Layer::ParseFromString instead of google::protobuf::TextFormat::ParseFromString to parse the mvn_param, so the prototxt was ignored. 3) Something else? I don't remember at the moment.

I created 2964 where I first fixed only the tests per @longjon suggestion, which should show about half the tests failing. I'll cherry-pick the MVNLayer fixes from 1979.

@cdoersch
Copy link
Contributor Author

Looks like #2964 incorporates all these changes. Closing.

@cdoersch cdoersch closed this Aug 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments