Skip to content

Comments

Fixed bilinear filler#5713

Merged
shelhamer merged 1 commit intoBVLC:masterfrom
Noiredd:filler
Oct 3, 2017
Merged

Fixed bilinear filler#5713
shelhamer merged 1 commit intoBVLC:masterfrom
Noiredd:filler

Conversation

@Noiredd
Copy link
Member

@Noiredd Noiredd commented Jun 27, 2017

BilinearFiller seems broken for some small values of kernel_size, for example 3, 6, 7, 8 10. For size 3 it generates this:

[ 0.0625  0.1875  0.1875]
[ 0.1875  0.5625  0.5625]
[ 0.1875  0.5625  0.5625]

instead of something more symmetric. I feel like it's an issue, since this is the exact size of filter needed for upsampling by factor of 2. (see below)
You can verify the problem exists, just run the following code on this network.

This PR corrects the crucial line in filler code.
BTW, this was noticed earlier in #4677, but due to a typo it never passed the basic tests. Besides, I noticed that this filler is never tested - unlike all others. Let me know if this is worth pursuing and I'll write a test or two in my spare time.

Additionally, the current implementation, quoting the comment from current master, is equivalent to the following call in Python with Scikit.Image:
out = skimage.transform.rescale(img, factor, mode='constant', cval=0)
Possibly I don't understand it, but it seems to me that it doesn't work like that. For example, with factor=2 we would have to create a deconv layer of stride: 2, kernel_size: 3 and pad: 1 - per formulas in the section quoted above. And the output of rescaling some image via scikit should be the same as if we forwarded it through this little net - correct? I tried to do that but the results differ - even the output shape is different for two methods: (3,7) for deconv, (4,8) for scikit. Contents are somewhat similar other than that. With my PR merged, they differ noticeably - possibly by fixing something I need, I'm breaking a functionality I don't understand?

EDIT (regarding compatibility with scikit): Indeed it was my mistake - hastily calculated the filter size. So the issue I'm describing does not concern scikit compatibility, neither my PR disrupts it. The fault still exists though - it just so happens that none of the broken filters are used for integer factor upsampling: factors 1-5 require kernels of sizes 4, 5, 8, 9 and 12 respectively, and all of those values work well with the current filler implementation. However, for kernels of size 3, 6, 7, 10 (or in general: any size that is not representable by 4*n-1 or 4n-2 where n is a natural number) generated filters are wrong.

Task list:

  • fix bilinear filler
  • sort out compatibility with scikit
  • write unit tests

@Noiredd Noiredd changed the title Fixed bilinear filler [WIP ]Fixed bilinear filler Jun 27, 2017
@Noiredd Noiredd changed the title [WIP ]Fixed bilinear filler [WIP] Fixed bilinear filler Jun 27, 2017
@shelhamer
Copy link
Member

Thanks for catching this! It is obviously wrong.

I have always done bilinear weights by net surgery in python, and somehow did not check the right cases in the C++ implementation. I agree that this should be tested, as the other filters are, and the tests should cover both even and odd filter sizes.

I'll happily merge this once the tests are included.

@shelhamer shelhamer added the bug label Jun 29, 2017
@Noiredd
Copy link
Member Author

Noiredd commented Oct 2, 2017

Finally found the time to get back to this and write the unit tests. I arbitrarily chose filter sizes of 6 and 7 because they 1) cover both odd and even cases, 2) are examples of problems in the current implementation. If something more elaborate is needed - just ping me.

By the way, I found an error in ConstantFillerTest, where the assertion was EXPECT_GE instead of EXPECT_EQ, and took the liberty of correcting that too. If we'd rather have this as a separate PR for clarity - let me know.

@Noiredd Noiredd changed the title [WIP] Fixed bilinear filler Fixed bilinear filler Oct 2, 2017
@shelhamer shelhamer merged commit ef2eb4b into BVLC:master Oct 3, 2017
@shelhamer
Copy link
Member

Thanks for the fix and tests @Noiredd! I noted the correction to the constant filler test in the merge commit message to save you the trouble of splitting it off.

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.

2 participants