Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support valid padding in Conv2D #16

Closed

Conversation

AdamHillier
Copy link

For some work I was doing recently I needed to verify networks that had convolutional layers with valid padding, and so I made some light modifications to support such layers in MIPVerify. What do you think of this approach?

@vtjeng
Copy link
Owner

vtjeng commented Jun 19, 2019

Thanks for the PR! Looks like a good approach overall 😄 I'm going to take a closer look as soon as I can --- hopefully within the week --- and leave some more specific comments.

Copy link
Owner

@vtjeng vtjeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your PR!

In addition to my inline comment, would you mind adding tests here for the new padding type? It would looks something like this

    @testset "conv2d with 'valid' padding" begin
        input = reshape([1:1:25;], (1, 5, 5, 1)
        filter = ones(3, 3, 1, 1)  # basic tensor containing all ones
        bias = [0]
        true_output_raw = [
            63 72 81;
            108 117 126;
            153 162 171
        ]
        true_output = reshape(transpose(true_output_raw), (1, 3, 3, 1))
        p = Conv2d(filter, bias)
        @testset "Numerical Input, Numerical Layer Parameters" begin
            evaluated_output = MIPVerify.conv2d(input, p)
            @test evaluated_output == true_output
        end

# add additional tests for other examples of filter dimensions --- I'd appreciate if you could do `filter` and `input` of even dimensions to verify that they're behaving as we expect.

You should be able to run this test file locally via

$ julia ~/.julia/v0.6/MIPVerify/test/net_components/layers/conv2d.jl
# modify path as appropriate

The tests will also be run when you push your code via our CI hooks.

filter_height_offset = round(Int, filter_height/2, RoundUp)
filter_width_offset = round(Int, filter_width/2, RoundUp)
else
@assert(padding == valid)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than this assertion, what about

Suggested change
@assert(padding == valid)
if padding == same
...
elseif padding == valid
...
else
error("Unknown padding type $padding")
end

@IsSeck
Copy link

IsSeck commented Apr 19, 2020

Hello,

I apologize if this is not the place for my question.

I wanted to know if padding=valid is implemented and if it is how to use it with get_conv_params if possible? Thank you

@vtjeng
Copy link
Owner

vtjeng commented Apr 19, 2020

Thank you --- this is the right place for your question.

Unfortunately, I haven't had the chance to merge padding=valid in just yet; it conflicts partially with a refactor that I merged. You should be able to modify the source code of the package on disk to implement this, and you may be able to try to merge in the changes in this PR (although you'll have to resolve the merge conflict).

@samuelemarro
Copy link
Contributor

Valid padding support would be really helpful. If you need help with the pull request, I'd be glad to help!

@vtjeng
Copy link
Owner

vtjeng commented Apr 21, 2020

@samuelemarro, that'll be much appreciated. You should be able to use much of Adam's code (feel free to open a new pull request, we'll make sure that Adam is properly credited). It would be best if you could add testing as mentioned above. #16 (review)

(Also, if you are planning to use this for your own networks, a sanity check on real values --- e.g. forward propagating input through the network and making sure it matches what you get in Pytorch / Tensorflow --- would be another great test to make sure we are not getting the convention subtly wrong).

@AdamHillier
Copy link
Author

Hi all, I'm very sorry that I never got round to coming back to this. @samuelemarro please be my guest to move forward with this code! I'll close this for now and you can open a new PR as @vtjeng suggests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants