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

Matrix subview support #1435

Merged
merged 14 commits into from Jul 2, 2018

Conversation

Projects
None yet
3 participants
@haritha1313
Contributor

haritha1313 commented Jun 16, 2018

No description provided.

haritha1313 added some commits Jun 16, 2018

haritha1313 added some commits Jun 17, 2018

@zoq

zoq approved these changes Jun 18, 2018

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

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jun 19, 2018

Member

In the latest blog post you said:

I am facing a few issues when using the newly created subview layer is merged with existing neural network models, which I am trying to debug right now.

Can you comment on that point, perhaps we can provide some input on this one.

Member

zoq commented Jun 19, 2018

In the latest blog post you said:

I am facing a few issues when using the newly created subview layer is merged with existing neural network models, which I am trying to debug right now.

Can you comment on that point, perhaps we can provide some input on this one.

@haritha1313

This comment has been minimized.

Show comment
Hide comment
@haritha1313

haritha1313 Jun 19, 2018

Contributor

@zoq It was a slight mistake. I thought it was the introduction of subview layer which caused the trouble, and was trying to debug that. But the issue was with the inSize and outSize parameters on introducing Embedding layer. It has been solved now. Thanks for offering help. I think the CreateModel() method is complete now. I think I'll go on and make a PR with the implemented methods so that you can stay updated. It would be great if #1427 was merged before that, unless there is anything pending there. :)

Contributor

haritha1313 commented Jun 19, 2018

@zoq It was a slight mistake. I thought it was the introduction of subview layer which caused the trouble, and was trying to debug that. But the issue was with the inSize and outSize parameters on introducing Embedding layer. It has been solved now. Thanks for offering help. I think the CreateModel() method is complete now. I think I'll go on and make a PR with the implemented methods so that you can stay updated. It would be great if #1427 was merged before that, unless there is anything pending there. :)

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jun 19, 2018

Member

Okay, #1427 is merged, there is just one last issue we should think about before merging this one as well.

Currently, this layer doesn't support batches, which I think would be great if we like to accelerate the training process. So in case of batches, the input is a matrix were each col represents another sample. An easy solution would be iterate over the input, but I don't see an easy way which allows us to differentiate between the batch and non-batch input which could be a matrix as well. One solution would be to let the user provide the input size, let me know what you think.

Member

zoq commented Jun 19, 2018

Okay, #1427 is merged, there is just one last issue we should think about before merging this one as well.

Currently, this layer doesn't support batches, which I think would be great if we like to accelerate the training process. So in case of batches, the input is a matrix were each col represents another sample. An easy solution would be iterate over the input, but I don't see an easy way which allows us to differentiate between the batch and non-batch input which could be a matrix as well. One solution would be to let the user provide the input size, let me know what you think.

@haritha1313

This comment has been minimized.

Show comment
Hide comment
@haritha1313

haritha1313 Jun 20, 2018

Contributor

Referring to other layers implemented, input size which user provides is of form (inSize, outSize). So does this assume that each sample is a vector, making the net input a matrix? I assume that if there is batch input where each sample is of matrix form, the total input will be arma::cube. I have a doubt regarding this. Is the point that each sample corresponds to each column true for this case too i.e subcube along a column is a sample? Or will slice of cube represent a sample?

As for distinguishing between batch and non-batch, as you said, we can add two parameters num_rows, num_cols (num_cols = 1 for vector input) in the constructor for input size, and iterate over each sample.

Contributor

haritha1313 commented Jun 20, 2018

Referring to other layers implemented, input size which user provides is of form (inSize, outSize). So does this assume that each sample is a vector, making the net input a matrix? I assume that if there is batch input where each sample is of matrix form, the total input will be arma::cube. I have a doubt regarding this. Is the point that each sample corresponds to each column true for this case too i.e subcube along a column is a sample? Or will slice of cube represent a sample?

As for distinguishing between batch and non-batch, as you said, we can add two parameters num_rows, num_cols (num_cols = 1 for vector input) in the constructor for input size, and iterate over each sample.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jun 20, 2018

Member

If a single input sample is a matrix, it will be vectorized internally, so that each layer can expect that each col represents another sample. In case of the RNN class you can pass an arma::cube as input, were each slice represents another time step but in this case the input is also transformed as well.

So, we could say that a user has to specify the inSize, I think outSize isn't needed since we can derive the rest from the other parameters. In most of the cases inSize is 1, and if the input is a matrix inSize > 1, this should be a rare case, and I don't think we will need it for the ann modules right now.

We could also call the parameter width and height in this case width is the same as inSize but I don't think we need height. What do you think?

Member

zoq commented Jun 20, 2018

If a single input sample is a matrix, it will be vectorized internally, so that each layer can expect that each col represents another sample. In case of the RNN class you can pass an arma::cube as input, were each slice represents another time step but in this case the input is also transformed as well.

So, we could say that a user has to specify the inSize, I think outSize isn't needed since we can derive the rest from the other parameters. In most of the cases inSize is 1, and if the input is a matrix inSize > 1, this should be a rare case, and I don't think we will need it for the ann modules right now.

We could also call the parameter width and height in this case width is the same as inSize but I don't think we need height. What do you think?

@haritha1313

This comment has been minimized.

Show comment
Hide comment
@haritha1313

haritha1313 Jun 20, 2018

Contributor

Thanks for helping me understand. So we just have to iterate input.n_rows/inSize times and create subviews of input using the existing method. Is that right?

Contributor

haritha1313 commented Jun 20, 2018

Thanks for helping me understand. So we just have to iterate input.n_rows/inSize times and create subviews of input using the existing method. Is that right?

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jun 20, 2018

Member

input.n_cols / inSize times

here is a simple example for batchSize = 1 and inSize = 1 it would look like

input = arma::ones(10, 1);

Subview(0, 4)

would return the first half of the input vector of size:

output = arma::ones(5, 1);

if we use a batchSize > 1 e.g. 2

the input looks like:

input = arma::ones(10, 2);

inSize is still 1, so the output would be of size

output = arma::ones(5, 2);

if someone does use a matrix as input e.g.

arma::mat(10, 10)

it will be transformed to:

arma::mat(10 * 10, 1)

so the output of the Subview layer would be:

size_t batchSize = input.n_cols / inSize

output.set_size(... (depends on the layer parameter), batchSize);

for (size_t i = 0; i < batchSize; i++)
{
  if (beginRow == 0 && endRow == (input.n_rows - 1))
  {
    output.col(i) = arma::mat(&input(beginCol * input.n_rows), input.n_rows * endCol - beginCol + 1, 1, false);
  }
  else
  {
    output.col(i) = arma::vectorise(input.submat(beginRow, beginCol, endRow, endCol));
  }
}

I hope this is useful, let me know if I should clarify anything.

Member

zoq commented Jun 20, 2018

input.n_cols / inSize times

here is a simple example for batchSize = 1 and inSize = 1 it would look like

input = arma::ones(10, 1);

Subview(0, 4)

would return the first half of the input vector of size:

output = arma::ones(5, 1);

if we use a batchSize > 1 e.g. 2

the input looks like:

input = arma::ones(10, 2);

inSize is still 1, so the output would be of size

output = arma::ones(5, 2);

if someone does use a matrix as input e.g.

arma::mat(10, 10)

it will be transformed to:

arma::mat(10 * 10, 1)

so the output of the Subview layer would be:

size_t batchSize = input.n_cols / inSize

output.set_size(... (depends on the layer parameter), batchSize);

for (size_t i = 0; i < batchSize; i++)
{
  if (beginRow == 0 && endRow == (input.n_rows - 1))
  {
    output.col(i) = arma::mat(&input(beginCol * input.n_rows), input.n_rows * endCol - beginCol + 1, 1, false);
  }
  else
  {
    output.col(i) = arma::vectorise(input.submat(beginRow, beginCol, endRow, endCol));
  }
}

I hope this is useful, let me know if I should clarify anything.

@haritha1313

This comment has been minimized.

Show comment
Hide comment
@haritha1313

haritha1313 Jun 20, 2018

Contributor

Sorry. input.n_cols was what I meant. Made a mistake while typing. :D . Thanks for the much useful explanation.

Contributor

haritha1313 commented Jun 20, 2018

Sorry. input.n_cols was what I meant. Made a mistake while typing. :D . Thanks for the much useful explanation.

haritha1313 added some commits Jun 21, 2018

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jun 22, 2018

Member

Okay, just one more thing, do you think we could add a batch test?

Member

zoq commented Jun 22, 2018

Okay, just one more thing, do you think we could add a batch test?

haritha1313 added some commits Jun 23, 2018

@rcurtin

rcurtin approved these changes Jul 1, 2018

With the aliasing discussion fixed, everything here looks good to me. 👍

@zoq

zoq approved these changes Jul 1, 2018

Looks good to me as well, thanks for looking into the issues.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jul 1, 2018

Member

Let me know if you need help with the merge conflicts.

Member

zoq commented Jul 1, 2018

Let me know if you need help with the merge conflicts.

haritha1313 added some commits Jul 2, 2018

@zoq zoq merged commit b9a4eb8 into mlpack:master Jul 2, 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.

Show comment
Hide comment
@zoq

zoq Jul 2, 2018

Member

Thanks again for looking into all those issues.

Member

zoq commented Jul 2, 2018

Thanks again for looking into all those issues.

@haritha1313 haritha1313 deleted the haritha1313:subviewMat branch Jul 7, 2018

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