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

Add support for residual blocks. #1594

Merged
merged 3 commits into from Jan 11, 2019

Conversation

Projects
None yet
4 participants
@akhandait
Copy link
Member

commented Dec 12, 2018

I also added the DeleteModules() function to the Sequential object as there was no way to free memory of added layers(with no pointers) when model == true.

@akhandait akhandait force-pushed the akhandait:res_block_support branch from 9d56a29 to 2a6da53 Dec 12, 2018

@rcurtin
Copy link
Member

left a comment

Nice improvement, looks good to me. 👍 (but I am not a huge expert on residual networks so others may have better input here)

@@ -137,7 +152,7 @@ class Sequential
//! Modify the output parameter.
arma::mat& OutputParameter() { return outputParameter; }

//! Get the delta.e
//! Get the delta.

This comment has been minimized.

Copy link
@rcurtin

rcurtin Dec 14, 2018

Member

I wonder how that extra e got there :)

This comment has been minimized.

Copy link
@akhandait

akhandait Dec 17, 2018

Author Member

Haha :)

Show resolved Hide resolved src/mlpack/methods/ann/layer/sequential.hpp Outdated
Show resolved Hide resolved src/mlpack/tests/ann_layer_test.cpp
Show resolved Hide resolved src/mlpack/methods/ann/layer/sequential_impl.hpp
@ShikharJ

This comment has been minimized.

Copy link
Member

commented Dec 14, 2018

@akhandait Nice! I'm all in favour for adding this feature in the existing framework. Please let me know if you feel it is ready for a review.

@rcurtin

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

Everything looks great from my end but I'll leave the approval for someone else given my relative unfamiliarity with residual layers. 👍

@ShikharJ

This comment has been minimized.

Copy link
Member

commented Dec 22, 2018

@akhandait Is there something you wish to add here or is it ready for reviewing? I'd like to make use of this in the CycleGAN PR.

@akhandait

This comment has been minimized.

Copy link
Member Author

commented Dec 23, 2018

@ShikharJ I don't have anything to add to this PR. It's ready for review. That's great!

@ShikharJ
Copy link
Member

left a comment

A few minor comments from my side.

Show resolved Hide resolved src/mlpack/methods/ann/layer/sequential_impl.hpp
Show resolved Hide resolved src/mlpack/tests/ann_layer_test.cpp Outdated
Show resolved Hide resolved src/mlpack/tests/ann_layer_test.cpp Outdated
Show resolved Hide resolved src/mlpack/tests/ann_layer_test.cpp Outdated
Show resolved Hide resolved src/mlpack/tests/ann_layer_test.cpp Outdated
delete sequential;
delete residual;
delete linearA;
delete linearB;

This comment has been minimized.

Copy link
@ShikharJ

ShikharJ Dec 23, 2018

Member

I think it should be possible to implement this test without using pointers, can you try exploring that, so that it remains consistent with the rest of the code?

This comment has been minimized.

Copy link
@zoq

zoq Dec 27, 2018

Member

hm, I can't comment on the comment below:

@akhandait Is the required change substantial? @zoq I'd like to know your take on this. I think it's better if we use lesser workarounds.

I think at this stage the solution is fine and necessary, if we can find a better solution later e.g. by using unique_ptr we can remove the part.

This comment has been minimized.

Copy link
@akhandait

akhandait Jan 4, 2019

Author Member

Yeah, I do think it's necessary for the time being.

This comment has been minimized.

Copy link
@ShikharJ

ShikharJ Jan 4, 2019

Member

@akhandait Okay, do you mind opening an issue for this, so that we keep track of this change?

This comment has been minimized.

Copy link
@akhandait

akhandait Jan 4, 2019

Author Member

Sure, I think that makes sense in this case. I also think we should get this merged for now and have the issue as a platform for further change.

Show resolved Hide resolved src/mlpack/tests/ann_layer_test.cpp
@ShikharJ

This comment has been minimized.

Copy link
Member

commented Dec 31, 2018

@akhandait Any update on this?

@ShikharJ
Copy link
Member

left a comment

I think this is very close to completion, I think if you can handle the comment and the test case, this should be ready for merging.

Show resolved Hide resolved src/mlpack/methods/ann/layer/sequential.hpp Outdated

@akhandait akhandait force-pushed the akhandait:res_block_support branch from bed8b14 to 13104b1 Jan 4, 2019

@akhandait

This comment has been minimized.

Copy link
Member Author

commented Jan 4, 2019

As @ShikharJ suggested, I created an issue #1631 to keep track of what we do with the memory issue.

@ShikharJ

This comment has been minimized.

Copy link
Member

commented Jan 5, 2019

@akhandait I also left a comment here. Do you think you can handle this? If not, I'll approve this as is and let it sit for a couple of days before merging in. I'll make the changes out in the CycleGAN PR itself.

@akhandait

This comment has been minimized.

Copy link
Member Author

commented Jan 6, 2019

@ShikharJ That issue stems from the issue I opened above. I am a little busy with some projects for now, If you can think it over in your CycleGAN PR, that'll will be great. Thanks!

@ShikharJ
Copy link
Member

left a comment

I'll let this sit for a couple of days, so that others may review it. Thanks for the contribution.

@akhandait

This comment has been minimized.

Copy link
Member Author

commented Jan 8, 2019

I will correct the build issue in a day or two.

@akhandait

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2019

Hmm, I restarted the build and the travis check passed. I guess it was some other problem as I also had no problems building this on my system.
I will wait 2 days if anybody wants to have a final look, especially over the last commit. Then I will merge this.

@ShikharJ

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

Must've been an occasional Travis issue then (I personally ran it like 5 times, and even Ryan did). I'll merge this, since it has already been around for some time.

@ShikharJ ShikharJ merged commit 07252cc into mlpack:master Jan 11, 2019

6 checks passed

LaTeX Documentation Checks Build finished.
Details
Memory Checks Build finished.
Details
Static Code Analysis Checks Build finished.
Details
Style Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.