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

subview layer addition #1428

Merged
merged 6 commits into from Jun 14, 2018

Conversation

Projects
None yet
3 participants
@haritha1313
Copy link
Contributor

haritha1313 commented Jun 10, 2018

No description provided.

haritha1313 added some commits Jun 10, 2018

template<typename InputType, typename OutputType>
void Forward(InputType&& input, OutputType&& output)
{
// Check if input has been selected as required.

This comment has been minimized.

@zoq

zoq Jun 11, 2018

Member

I was wondering if we could use one of armadillo's advanced constructors to reuse the memory, something like:

output = arma::Mat<ElemType>(input.memptr() + begin, 1, end - begin, false, false);

This comment has been minimized.

@haritha1313

haritha1313 Jun 11, 2018

Author Contributor

Ofcourse, I'll edit it. Just one doubt, shouldn't the n_rows parameter be (end-begin) and n_cols be input.n_cols, considering we are passing matrices and not just vectors?

This comment has been minimized.

@zoq

zoq Jun 11, 2018

Member

You are right, just thought about vectors.

}
arma::Mat<eT> derivative;
IdentityFunction::Deriv(input, derivative);
g = gy % derivative;

This comment has been minimized.

@zoq

zoq Jun 11, 2018

Member

I think we can just forward the gy so g = gy, that's what the identity function does anyway, maybe I missed anything?

This comment has been minimized.

@haritha1313

haritha1313 Jun 11, 2018

Author Contributor

You are right, just took a look at identityfunction.hpp.

* @param begin Start index.
* @param end End index.
*/
Subview(const arma::uword begin = 0, const arma::uword end = 0):

This comment has been minimized.

@zoq

zoq Jun 12, 2018

Member

To be consistent with the rest of the codebase, can we use size_t instead of uword.

// Check if input has been selected as required.
if ((input.n_rows != (end-begin+1)) && (end != 0))
{
arma::uword cols = input.n_cols;

This comment has been minimized.

@zoq

zoq Jun 12, 2018

Member

I think we could directly use input.n_cols.

arma::Mat<eT>&& g)
{
// Check if input has been selected as required.
if ((input.n_rows != (end-begin+1)) && (end != 0))

This comment has been minimized.

@zoq

zoq Jun 12, 2018

Member

Do you think we can remove the if block? Looks like the input isn't used anyway.

// Test the Forward function.
input = arma::ones(20, 1);
module.Forward(std::move(input), std::move(output));
BOOST_REQUIRE_EQUAL(output.n_rows, 10);

This comment has been minimized.

@zoq

zoq Jun 12, 2018

Member

It might be good to test the check the output as well. We could use http://arma.sourceforge.net/docs.html#linspace to create an input of the form [1, 2, 3, 4, 5, 6, 7, 8, 9, 10], we can also hardcode the input and check if the output does match with the given index. In this case it might be a good idea to check different start and end points.

haritha1313 added some commits Jun 12, 2018

@zoq

zoq approved these changes Jun 12, 2018

Copy link
Member

zoq left a comment

This looks good to me. I'll let this sit for 2 days before I'll merge this in, to give anyone else a chance to take a look at the code.

@zoq zoq merged commit 86bcca6 into mlpack:master Jun 14, 2018

5 checks passed

Memory Checks
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
@zoq

This comment has been minimized.

Copy link
Member

zoq commented Jun 14, 2018

Thanks for another great contribution.

@haritha1313 haritha1313 deleted the haritha1313:SubviewLayer branch Jun 14, 2018

@rcurtin
Copy link
Member

rcurtin left a comment

I know I am late with the comments, sorry about that. :) Everything looks good to me, just some quick comments about name and serialization. I don't see them as huge issues (maybe the serialization bit is a bug) so if you want to leave it as-is, no issues from my side.

template<typename Archive>
void serialize(Archive& /* ar */, const unsigned int /* version */)
{
/* Nothing to do here */

This comment has been minimized.

@rcurtin

rcurtin Jun 14, 2018

Member

I know I am late to the party here, but should we serialize begin and end?

This comment has been minimized.

@zoq

zoq Jun 14, 2018

Member

Right, nice catch, let me fix that.

* Create the Subview layer object using the specified range of input to accept.
*
* @param begin Start index.
* @param end End index.

This comment has been minimized.

@rcurtin

rcurtin Jun 14, 2018

Member

Looks like this works with 1D inputs, but it might be useful to call this Subview1D, and perhaps provide some other time a Subview2D layer, in case the input is matrix-shaped (not vector-shaped).

This comment has been minimized.

@zoq

zoq Jun 14, 2018

Member

Good point, it should be possible to support both in a single class, so I think we can still call this class Subview.

This comment has been minimized.

@haritha1313

haritha1313 Jun 14, 2018

Author Contributor

I think it does work for both vector and matrix shaped inputs. Maybe I understood something wrong, but I tried running it on matrices and it is being modified as required.

This comment has been minimized.

@rcurtin

rcurtin Jun 14, 2018

Member

Right, it works with both, but let's suppose I have a matrix of size 5x5 and what I want to do is take the middle 3x3 elements out of it. If we use the Armadillo advanced constructors we don't have a way to do that, since the middle 3x3 won't be contiguous in memory. In that situation we would have no choice but to extract the submatrix with submat(). (I guess it might be possible to pass an arma::subview type to the next layer as input, but I am not sure if that would actually work in the framework we have. I don't think that avoiding the subview copy in this situation is so important that it has to be dealt with right now though.)

This comment has been minimized.

@haritha1313

haritha1313 Jun 15, 2018

Author Contributor

Right, then we can go on with submat(). I suppose. @zoq Unfortunately I deleted the branch I was working on, so just let me know if I should restore it and revert the merge to make these changes.

This comment has been minimized.

@zoq

zoq Jun 15, 2018

Member

No need to revert the changes, we can open a new PR to do the modification.

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.