Added accessor method implementations for ANN layers #2269

iamshnoo · 2020-03-09T14:00:19Z

Solves the issue described in #2258
The final comment by me in that issue gives a high-level overview of the changes I have made here.

@zoq @sreenikSS Could you guys give me some feedback about this PR? Thanks.

mlpack-bot · 2020-03-09T14:00:22Z

Thanks for opening your first pull request in this repository! Someone will review it when they have a chance. In the mean time, please be sure that you've handled the following things, to make the review process quicker and easier:

All code should follow the style guide
Documentation added for any new functionality
Tests added for any new functionality
Tests that are added follow the testing guide
Headers and license information added to the top of any new code files
HISTORY.md updated if the changes are big or user-facing
All CI checks should be passing

Thank you again for your contributions! 👍

typo fix in recurrent_attention layer

Improving Contributing guidlines

Removed r-value references from remaining functions [and some style fixes]

Fixed markdown pipe symbol issue

zoq · 2020-03-13T19:20:10Z

src/mlpack/methods/ann/layer/add.hpp

  OutputDataType& Gradient() { return gradient; }

+  //! Get the output size.
+  size_t const& OutputSize() const { return outSize; }


Can you use the same style as used here:

https://github.com/mlpack/mlpack/blob/master/src/mlpack/methods/ann/layer/linear.hpp#L119

zoq · 2020-03-13T19:20:33Z

src/mlpack/methods/ann/layer/batch_norm.hpp

  OutputDataType TrainingVariance() { return runningVariance / count; }

+  //! Get the number of input units.
+  size_t const& InSize() const { return size; }


See comment above, I think we should use the same style across the codebase.

Okay! Will update these changes in a commit probably a few hours from now.

* soft shrinkage activation function * changes * new changes * new changes * void Fn complete * comments * changes * changes * lambda as param * added derivate also * changes * complete function * changes in ss function * new changes * soft shrinkage activation function * changes * new changes * new changes * void Fn complete * comments * changes * changes * lambda as param * added derivate also * changes * complete function * changes in ss function * new changes * style corrections * style corrections 2 * corrections * corrections * final corrections * base layer changes * cmake softshrink function * softshrink.hpp in layer * softshrink_impl.hpp added * layer_types.hpp updated * updating activation_functions_test.cpp * soft shrink changes * commit * style corrections * style changes * changes style * small style changes * changes * style checks * final checks * changes * doxygen syntax for comment * soft shrink comments * style checks for 80 characters * dummy commit * softshrink.hpp to softshrink_impl.hpp fn to forward * bug fixes in build * style fixes * updating lambda definition Co-Authored-By: Ryan Birmingham <[email protected]> * Use _Internal module, not util. * Print code snippets in ```julia blocks. * Always pass 64-bit integers back and forth. * Additional i386 fixes. * Remove unneeded REQUIRE. * Add docstrings to the mlpack module. * New NumFOCUS badge mockup * Match style of other badges * Minor changes reflecting discusssion * Use Csize_t instead of uint64_t, and UInt/Int. * Oops, fix declaration of s. * Switch int64_t to int. * Remove inaccurate comments. * Fix a few other minor issues. * Redirected badge and removed info about NUMfocus * Reverted deletion and added attribution line * added @tparam and rvalue correction according to recent refactor pr Co-authored-by: Ryan Birmingham <[email protected]> Co-authored-by: Ryan Curtin <[email protected]> Co-authored-by: Sriram <[email protected]>

Use Cint to match C++'s int type, not Julia's Int.

sreenikSS

Good job. However, keep in mind as Ryan as already said, that you'll need to pull master and then make these changes to avoid a conflict.

sreenikSS · 2020-03-13T20:28:08Z

src/mlpack/methods/ann/layer/highway.hpp

+  //! Get the value of model parameter.
+  bool const& Model() const { return model;}


You may consider changing the name of the function as Model() already exists and returns the network variable.

I also don't think that having a getter function for the private variable model is absolutely necessary.

Okay! I see your point. I will remove the Model() getters in the next commit.

sreenikSS · 2020-03-14T02:57:03Z

src/mlpack/methods/ann/layer/multiply_merge.hpp

+  //! Get the value of model parameter.
+  bool const& Model() const { return model; }


Same as above.

sreenikSS · 2020-03-14T03:00:32Z

src/mlpack/methods/ann/layer/multiply_merge.hpp

+  //! Get the value of run parameter.
+  bool const& Run() const { return run; }
+


Looks like run is not something that might benefit with a getter function.
On the other hand, having a getter coupled with a modifier might make sense if the user wants to manually shift between forward and backward. Any thoughts on this @zoq?

I am going ahead and adding a setter for this now. If required I can remove it in a later commit.

sreenikSS · 2020-03-14T03:06:42Z

src/mlpack/methods/ann/layer/recurrent.hpp

+  //! Get the number of steps to backpropagate through time.
+  size_t const& Rho() const { return rho; }
+


This is fine, however, little can be known by getting the rho alone. I think obtaining the other constructor parameters would be useful too.

So, there are 4 other variables in the constructor, namely const StartModuleType& start, const InputModuleType& input, const FeedbackModuleType& feedback and const TransferModuleType& transfer. I did not write getter functions for them because the data types that will be returned in that case are not usually returned in any other layers. Most probably this is because, these modules are pushed into the network and then returned via the getter for network. Let me know if I should still be returning them via getters and if so, what should be the style I need to follow!

Yes you are right, I had overlooked that. No need to write separate getters for these since we have a getter for the network itself.

sreenikSS · 2020-03-14T03:08:05Z

src/mlpack/methods/ann/layer/recurrent_attention.hpp

+  //! Get the module output size.
+  size_t const& OutSize() const { return outSize; }
+
+  //! Get the number of steps to backpropagate through time.
+  size_t const& Rho() const { return rho; }
+


Same as as above, i.e., the other constructor parameters can have getters too.

Same reason as the one I specified above. rnnModule and actionModule are pushed back into the network here. And then we are returning the network here.

Same as above

sreenikSS · 2020-03-14T03:10:09Z

src/mlpack/methods/ann/layer/sequential.hpp

+  //! Get the value of the model parameter.
+  bool const& Model() const { return model; }
+


Same as the changes suggested in highway.hpp.

sreenikSS · 2020-03-14T03:11:03Z

src/mlpack/methods/ann/layer/subview.hpp

+  //! Get the width of each sample.
+  size_t const& InSize() const { return inSize; }
+
+  //! Get the starting row index of subview vector or matrix.
+  size_t const& BeginRow() const { return beginRow; }
+
+  //! Get the ending row index of subview vector or matrix.
+  size_t const& EndRow() const { return endRow; }
+
+  //! Get the starting column index of subview vector or matrix.
+  size_t const& BeginCol() const { return beginCol; }
+
+  //! Get the ending column index of subview vector or matrix.
+  size_t const& EndCol() const { return endCol; }
+


Might as well add modifiers for these.

…/mlpack into accessor_implementation

iamshnoo · 2020-03-14T15:58:12Z

Just a comment to avoid possible confusion. I am new to git. So, in case the commit history looks weird, my bad :) Also, I am not yet done with implementing all the changes requested. I am working on that commit as of now. And it shouldn't be long before I complete it and push.

sreenikSS · 2020-03-16T18:29:31Z

@iamshnoo it is currently very difficult to review the changes because the changes that you have pulled have made it to your commits as well. So reviewing your code separately would be a big hassle since there are a lot of files that have been changed (you can take a look at the Files Changed tab).

Could you just do something like create a new branch and copy-paste-replace the files where you have made changes and submit a new PR (just make sure there are no new changes to those files by other people after the last time you modified them)? Git might offer a better way to do it but this is what comes to my mind right now.

geekypathak21 · 2020-03-16T19:22:02Z

@sreenikSS I think updating the master branch will also work for this problem. I was also dealing with the same issue but when I updated master branch of my forked repo this issue was solved
Just do.

git checkout master
git remote add upstream https://github.com/mlpack/mlpack.git
git pull upstream master
git push

iamshnoo · 2020-03-16T20:44:37Z

Thanks @himanshupathak21061998 @sreenikSS for your suggestions.
I have created #2307 and hopefully it is correct this time :)
Maybe you guys would like to have a look at the changes there?

Also, should I be closing this PR as only the other one is now relevant?

sreenikSS · 2020-03-17T13:24:19Z

@iamshnoo you can close this one.

iamshnoo · 2020-03-17T14:50:50Z

Okay! Sure.

rcurtin and others added 26 commits March 3, 2020 17:56

Use _Internal module, not util.

4cff789

Print code snippets in ```julia blocks.

c985600

Always pass 64-bit integers back and forth.

6432555

Additional i386 fixes.

4d45e05

Remove unneeded REQUIRE.

d544771

Add docstrings to the mlpack module.

0621165

New NumFOCUS badge mockup

466e00b

Match style of other badges

573b350

Refactor to avoid rvalue references.

ffa6d8a

Merge remote-tracking branch 'origin/master' into ann-rvalue-reference

d90f1f0

Fix style issues.

b5d00ed

Minor changes reflecting discusssion

c4b3a7f

Add note for waiting time and review policy

02fa559

Use Csize_t instead of uint64_t, and UInt/Int.

d730879

Oops, fix declaration of s.

758cb5a

Switch int64_t to int.

c85d7ec

Remove inaccurate comments.

7ddf7ba

Fix a few other minor issues.

cab675b

Added Tests for MaxPooling

e38ee12

Changes in Pooling Layers

2a3cd35

Typo fix.

98b09fd

Style Fix

2dd1ad8

Add offset

d8c10c4

links for references

03a924c

fixed formatting

012011e

Added accessor method implementations for ANN layers

1a34f63

mlpack-bot bot added s: needs review s: unanswered s: unlabeled labels Mar 9, 2020

rcurtin and others added 11 commits March 12, 2020 21:04

Only add test if BUILD_JULIA_BINDINGS is specified.

3f07bcf

Switch to correct size_t type.

4bb869a

typo fix

abe5764

Merge pull request #2285 from mrityunjay-tripathi/typo-fix

8510d08

typo fix in recurrent_attention layer

Removed r-value references and some style fixes

744ddc5

Improve contrib docs - repharasing

3c78cac

Update CONTRIBUTING.md

d1188bd

Merge pull request #2260 from jeffin143/contrib-guide

11d4786

Improving Contributing guidlines

Fix unchanged header definition (oops).

add98c7

Merge pull request #2292 from kartikdutt18/master

2fb1c2b

Removed r-value references from remaining functions [and some style fixes]

Merge pull request #2280 from adithya-tp/markdown-issue-fix

4057806

Fixed markdown pipe symbol issue

zoq reviewed Mar 13, 2020

View reviewed changes

ojhalakshya and others added 3 commits March 13, 2020 15:37

Merge pull request #2283 from rcurtin/julia-cint

5676942

Use Cint to match C++'s int type, not Julia's Int.

Added Deterministic() function

ba25b8e

sreenikSS requested changes Mar 14, 2020

View reviewed changes

iamshnoo added 3 commits March 14, 2020 20:35

Added accessor method implementations for ANN layers

aedb93f

Correct the build errors from previous build on PR

aa19bda

Merge branch 'accessor_implementation' of https://github.com/iamshnoo…

c8eb9b4

…/mlpack into accessor_implementation

Update files as per changes requested

b36893d

iamshnoo requested review from sreenikSS and zoq March 16, 2020 13:32

iamshnoo mentioned this pull request Mar 16, 2020

Accessor method implementations (continued) #2307

Closed

iamshnoo closed this Mar 17, 2020

		//! Get the value of model parameter.
		bool const& Model() const { return model;}

		//! Get the value of run parameter.
		bool const& Run() const { return run; }

		//! Get the number of steps to backpropagate through time.
		size_t const& Rho() const { return rho; }

		//! Get the value of the model parameter.
		bool const& Model() const { return model; }

Uh oh!

Added accessor method implementations for ANN layers #2269

Added accessor method implementations for ANN layers #2269

Uh oh!

Conversation

iamshnoo commented Mar 9, 2020

Uh oh!

mlpack-bot bot commented Mar 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sreenikSS left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamshnoo commented Mar 14, 2020

Uh oh!

sreenikSS commented Mar 16, 2020

Uh oh!

geekypathak21 commented Mar 16, 2020

Uh oh!

iamshnoo commented Mar 16, 2020

Uh oh!

sreenikSS commented Mar 17, 2020

Uh oh!

iamshnoo commented Mar 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

15 participants