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

ResizeLayer implementation #1098

Closed
wants to merge 23 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@kris-singh
Contributor

kris-singh commented Aug 19, 2017

No description provided.

@lozhnikov lozhnikov changed the title from Intial Commit to ResizeLayer implementation Aug 19, 2017

@kris-singh

This comment has been minimized.

Show comment
Hide comment
@kris-singh

kris-singh Aug 20, 2017

Contributor

Write now i am getting this as input. For 4x
screen shot 2017-08-20 at 6 34 09 pm

Contributor

kris-singh commented Aug 20, 2017

Write now i am getting this as input. For 4x
screen shot 2017-08-20 at 6 34 09 pm

@lozhnikov

This comment has been minimized.

Show comment
Hide comment
@lozhnikov

lozhnikov Aug 20, 2017

Contributor

hmm... Why the second image has grid-like artifacts?

Contributor

lozhnikov commented Aug 20, 2017

hmm... Why the second image has grid-like artifacts?

@kris-singh

This comment has been minimized.

Show comment
Hide comment
@kris-singh

kris-singh Aug 20, 2017

Contributor

Since i am putting it on the same scale it seems there are grid-like artifacts. I will test this with a rgb image and check.

Contributor

kris-singh commented Aug 20, 2017

Since i am putting it on the same scale it seems there are grid-like artifacts. I will test this with a rgb image and check.

@kris-singh

This comment has been minimized.

Show comment
Hide comment
@kris-singh

kris-singh Aug 20, 2017

Contributor

This is the full sized image with the correct formula. The unzoomed image does not have the artifact though.
screen shot 2017-08-20 at 8 31 51 pm

Contributor

kris-singh commented Aug 20, 2017

This is the full sized image with the correct formula. The unzoomed image does not have the artifact though.
screen shot 2017-08-20 at 8 31 51 pm

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Aug 20, 2017

Member

For me it looks like you don't interpolate between the values, an easy test would be to pass an artificial image e.g. a 20x20 matrix filled with a constant value. My guess is that we end up with gaps.

Member

zoq commented Aug 20, 2017

For me it looks like you don't interpolate between the values, an easy test would be to pass an artificial image e.g. a 20x20 matrix filled with a constant value. My guess is that we end up with gaps.

@kris-singh

This comment has been minimized.

Show comment
Hide comment
@kris-singh

kris-singh Aug 20, 2017

Contributor

It works now.... the upper image is zoomed and the second image is the unzoomed from 800*800 --> 400 * 400
screen shot 2017-08-20 at 10 41 48 pm

Contributor

kris-singh commented Aug 20, 2017

It works now.... the upper image is zoomed and the second image is the unzoomed from 800*800 --> 400 * 400
screen shot 2017-08-20 at 10 41 48 pm

@lozhnikov

I haven't found serious issues. I think we have to add some tests. I guess it is reasonable to test the layer on a linear function and check the residual.

@kris-singh

This comment has been minimized.

Show comment
Hide comment
@kris-singh

kris-singh Aug 22, 2017

Contributor

@lozhnikov I think you can have a look now. The random forest test is failing.

Contributor

kris-singh commented Aug 22, 2017

@lozhnikov I think you can have a look now. The random forest test is failing.

void Resize<InterpolationType, InputDataType, OutputDataType>::Serialize(
Archive& ar, const unsigned int /* version */)
{
ar & data::CreateNVP(policy, "policy");

This comment has been minimized.

@lozhnikov

lozhnikov Aug 23, 2017

Contributor

In that case you should implement BiLinearFunction::Serialize().

@lozhnikov

lozhnikov Aug 23, 2017

Contributor

In that case you should implement BiLinearFunction::Serialize().

@kris-singh

This comment has been minimized.

Show comment
Hide comment
@kris-singh

kris-singh Aug 24, 2017

Contributor

This un zoomed image....
screen shot 2017-08-24 at 8 53 52 pm

Contributor

kris-singh commented Aug 24, 2017

This un zoomed image....
screen shot 2017-08-24 at 8 53 52 pm

kris-singh added some commits Aug 25, 2017

Fix style
;
@lozhnikov

I pointed out some issues.

* @param inRowSize: Number of input rows
* @param inColSize: Number of input columns.
* @param outRowSize: Number of output rows.
* @param outColSie: Number of output columns.

This comment has been minimized.

@lozhnikov

lozhnikov Aug 28, 2017

Contributor

You forgot to describe the depth argument.

@lozhnikov

lozhnikov Aug 28, 2017

Contributor

You forgot to describe the depth argument.

Show outdated Hide outdated src/mlpack/methods/ann/image_functions/bilinear_function.hpp
Show outdated Hide outdated src/mlpack/methods/ann/image_functions/bilinear_function.hpp
Show outdated Hide outdated src/mlpack/methods/ann/image_functions/bilinear_function.hpp
Show outdated Hide outdated src/mlpack/methods/ann/image_functions/bilinear_function.hpp
Show outdated Hide outdated src/mlpack/methods/ann/layer/layer_types.hpp
Show outdated Hide outdated src/mlpack/methods/ann/layer/resize.hpp
Show outdated Hide outdated src/mlpack/methods/ann/layer/resize_impl.hpp
Show outdated Hide outdated src/mlpack/tests/ann_layer_test.cpp
@kris-singh

This comment has been minimized.

Show comment
Hide comment
@kris-singh

kris-singh Aug 28, 2017

Contributor

I fixed all the comments. Also, i added the test for the 3d input but removed the 2d input test seems redundant to me.

Contributor

kris-singh commented Aug 28, 2017

I fixed all the comments. Also, i added the test for the 3d input but removed the 2d input test seems redundant to me.

kris-singh added some commits Aug 28, 2017

@lozhnikov

This comment has been minimized.

Show comment
Hide comment
@lozhnikov

lozhnikov Aug 29, 2017

Contributor

Fixed the api. But not sure about the comment.

I mean it is reasonable to add a comment that describes why the backward pass is just the inverse transform applied to the backpropogated error (in case of bilinear interpolation).

Contributor

lozhnikov commented Aug 29, 2017

Fixed the api. But not sure about the comment.

I mean it is reasonable to add a comment that describes why the backward pass is just the inverse transform applied to the backpropogated error (in case of bilinear interpolation).

@kris-singh

This comment has been minimized.

Show comment
Hide comment
@kris-singh

kris-singh Aug 29, 2017

Contributor

Could we restart travis on this one.

Contributor

kris-singh commented Aug 29, 2017

Could we restart travis on this one.

@lozhnikov

This comment has been minimized.

Show comment
Hide comment
@lozhnikov

lozhnikov Aug 29, 2017

Contributor

Moreover, I think it isn't too evident that the backward pass is just the inverse interpolation of the backpropogated error in case of bilinear interpolation. I guess we should add a comment in order to describe that.
I mean it is reasonable to add a comment that describes why the backward pass is just the inverse transform applied to the backpropogated error (in case of bilinear interpolation).

 /**
    * DownSample the given input.
    * This is equivalent to the Backward the pass for the layer.
    * Since, the layer dose not have any learnable parameters
    * we just have downsample the gradient to make it's size
    * compatible with the input size.
    *		

The backward pass should calculate the product of the derivative and the backpropogated error. I suggested to leave a comment that explains why the inverse transform applied to the backpropogated error is the same (it seems that's true in case of bilinear interpolation, but in general, the statement is incorrect for any layer without learnable parameters, e.g. that's incorrect for cubic splines).

It is possible to implement the backward transform in terms of the ActivationFunction API i.e.
g = derivative * error;
But the derivative isn't a diagonal matrix therefore the approach is cost-inefficient. However, we could use sparse matrices instead. @zoq How do you think, is it reasonable?

Contributor

lozhnikov commented Aug 29, 2017

Moreover, I think it isn't too evident that the backward pass is just the inverse interpolation of the backpropogated error in case of bilinear interpolation. I guess we should add a comment in order to describe that.
I mean it is reasonable to add a comment that describes why the backward pass is just the inverse transform applied to the backpropogated error (in case of bilinear interpolation).

 /**
    * DownSample the given input.
    * This is equivalent to the Backward the pass for the layer.
    * Since, the layer dose not have any learnable parameters
    * we just have downsample the gradient to make it's size
    * compatible with the input size.
    *		

The backward pass should calculate the product of the derivative and the backpropogated error. I suggested to leave a comment that explains why the inverse transform applied to the backpropogated error is the same (it seems that's true in case of bilinear interpolation, but in general, the statement is incorrect for any layer without learnable parameters, e.g. that's incorrect for cubic splines).

It is possible to implement the backward transform in terms of the ActivationFunction API i.e.
g = derivative * error;
But the derivative isn't a diagonal matrix therefore the approach is cost-inefficient. However, we could use sparse matrices instead. @zoq How do you think, is it reasonable?

@kris-singh

This comment has been minimized.

Show comment
Hide comment
@kris-singh

kris-singh Aug 30, 2017

Contributor

Well isn' this what we are exactly doing derivate in our case is 1 error is the gy(Which we have to downsample). g = 1 * gy.
I don't seem to get the problem.
@zoq Any updates on this

Contributor

kris-singh commented Aug 30, 2017

Well isn' this what we are exactly doing derivate in our case is 1 error is the gy(Which we have to downsample). g = 1 * gy.
I don't seem to get the problem.
@zoq Any updates on this

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Sep 3, 2017

Member

I'm not sure maybe I missed something , but instead of implementing the Interpolation as an Activation function we could just implement it as a Layer itself and remove the extra Resize layer. It looks like the Resize layer is just an extra wrapper around the interpolation function. I guess you could add an alias if you like to use Resize over BilinearInterpolation. Let me know what you think.

Member

zoq commented Sep 3, 2017

I'm not sure maybe I missed something , but instead of implementing the Interpolation as an Activation function we could just implement it as a Layer itself and remove the extra Resize layer. It looks like the Resize layer is just an extra wrapper around the interpolation function. I guess you could add an alias if you like to use Resize over BilinearInterpolation. Let me know what you think.

@kris-singh

This comment has been minimized.

Show comment
Hide comment
@kris-singh

kris-singh Sep 3, 2017

Contributor

@zoq Our idea here was that there could be a lot of interpolations function in the future hence we implemented in the manner of the base_layer.hpp or activation function file. This way it is more extendable.

Contributor

kris-singh commented Sep 3, 2017

@zoq Our idea here was that there could be a lot of interpolations function in the future hence we implemented in the manner of the base_layer.hpp or activation function file. This way it is more extendable.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Sep 3, 2017

Member

Not sure it's more extendable since all the Resize layer does is to wrap two functions. I think implementing the current function as an independent layer has another benefit since you could directly pass additional parameter instead of passing a policy instantiation. Like currently you would have to write

InterpolationFunction inter(/* function parameter */);
model.Add<Resize<> >(inter);

if we implement it as an indepent layer we could just write:

model.Add<InterpolationFunction<> >(/* function parameter */);

Anyway, this is just hypothetical, since the BilinearFunction does not provide an interface for additional parameter.

Member

zoq commented Sep 3, 2017

Not sure it's more extendable since all the Resize layer does is to wrap two functions. I think implementing the current function as an independent layer has another benefit since you could directly pass additional parameter instead of passing a policy instantiation. Like currently you would have to write

InterpolationFunction inter(/* function parameter */);
model.Add<Resize<> >(inter);

if we implement it as an indepent layer we could just write:

model.Add<InterpolationFunction<> >(/* function parameter */);

Anyway, this is just hypothetical, since the BilinearFunction does not provide an interface for additional parameter.

@kris-singh

This comment has been minimized.

Show comment
Hide comment
@kris-singh

kris-singh Sep 3, 2017

Contributor

@zoq I think what i am trying to say if you look at the linear interpolation, bilinear interpolation, bi cubic interpolation all there are represented as policies of the resize layer otherwise you would have to implement all these as a different layer. It makes sense to define them as policies since they are doing the same task.
Are you following my point?

Contributor

kris-singh commented Sep 3, 2017

@zoq I think what i am trying to say if you look at the linear interpolation, bilinear interpolation, bi cubic interpolation all there are represented as policies of the resize layer otherwise you would have to implement all these as a different layer. It makes sense to define them as policies since they are doing the same task.
Are you following my point?

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Sep 3, 2017

Member

I get your point, we did something similar with the optimizer classes, where we use SGD as a base class. But instead of providing a base class that takes an update policy we opted for a specilized class for each new Optimizer like Adam to improve the overall usability. So that you can use Adam(/* optimizer setting */) instead of

Adam policy(/* optimizer settings */)
SGD<Adam>(policy);

I think using Adam directly, instead of creating an instance first is easier. Anyway, as you pointed out we could implement the interpolation function as a policy, but maybe providing an independent layer is just easier to use? Unless, each interpolation function takes the same parameter, in this case, we should pass the parameter instead of an instance. Let me know what you think.

Member

zoq commented Sep 3, 2017

I get your point, we did something similar with the optimizer classes, where we use SGD as a base class. But instead of providing a base class that takes an update policy we opted for a specilized class for each new Optimizer like Adam to improve the overall usability. So that you can use Adam(/* optimizer setting */) instead of

Adam policy(/* optimizer settings */)
SGD<Adam>(policy);

I think using Adam directly, instead of creating an instance first is easier. Anyway, as you pointed out we could implement the interpolation function as a policy, but maybe providing an independent layer is just easier to use? Unless, each interpolation function takes the same parameter, in this case, we should pass the parameter instead of an instance. Let me know what you think.

@kris-singh

This comment has been minimized.

Show comment
Hide comment
@kris-singh

kris-singh Sep 4, 2017

Contributor

I think right now we can implement it as policy as you are suggesting by passing the same set of parameters. I would go ahead with that implementation i think if in future we come across an interpolation function we could always add the parameters that are being passed. I would go ahead and implement this. @lozhnikov do you have any comments here?

Contributor

kris-singh commented Sep 4, 2017

I think right now we can implement it as policy as you are suggesting by passing the same set of parameters. I would go ahead with that implementation i think if in future we come across an interpolation function we could always add the parameters that are being passed. I would go ahead and implement this. @lozhnikov do you have any comments here?

@lozhnikov

This comment has been minimized.

Show comment
Hide comment
@lozhnikov

lozhnikov Sep 4, 2017

Contributor

I agree with Marcus. Since ResizeLayer is just a wrapper I think it is reasonable to merge BiLinearInterpolation and ResizeLayer. I guess the only question we should answer is the following:
Are we going to use BiLinearInterpolation without ResizeLayer?
I think not since they do the same. Suppose we have a few interpolation functions. Anyway we have to add each instance to LayerTypes.

Contributor

lozhnikov commented Sep 4, 2017

I agree with Marcus. Since ResizeLayer is just a wrapper I think it is reasonable to merge BiLinearInterpolation and ResizeLayer. I guess the only question we should answer is the following:
Are we going to use BiLinearInterpolation without ResizeLayer?
I think not since they do the same. Suppose we have a few interpolation functions. Anyway we have to add each instance to LayerTypes.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Sep 5, 2017

Member

Forwarding the interpolation parameter sounds fine for me, as @lozhnikov pointed if we only use the interpolation function for the resize layer the difference is negligible.

Member

zoq commented Sep 5, 2017

Forwarding the interpolation parameter sounds fine for me, as @lozhnikov pointed if we only use the interpolation function for the resize layer the difference is negligible.

@ShikharJ

This comment has been minimized.

Show comment
Hide comment
@ShikharJ

ShikharJ Mar 7, 2018

Member

Thanks @kris-singh for your work here. This has been completed in #1238.
@zoq This can be closed now.

Member

ShikharJ commented Mar 7, 2018

Thanks @kris-singh for your work here. This has been completed in #1238.
@zoq This can be closed now.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Mar 9, 2018

Member

Thanks @kris-singh and @ShikharJ for the great work.

Member

zoq commented Mar 9, 2018

Thanks @kris-singh and @ShikharJ for the great work.

@zoq zoq closed this Mar 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment