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

[GSoC] Augmented RNN models - benchmarking framework #1005

Merged
merged 66 commits into from Aug 7, 2017

Conversation

Projects
None yet
4 participants
@17minutes
Contributor

17minutes commented May 23, 2017

This PR is part of my GSoC project "Augmented RNNs".
Imeplemented:

  • class CopyTask for evaluating models on the sequence copy problem, showcasing benchmarking framework;
  • unit test for it (a simple non-ML model that is hardcoded to copy the sequence required number of times is expected to ace the CopyTask).
@mlpack-jenkins

This comment has been minimized.

Show comment
Hide comment
@mlpack-jenkins

mlpack-jenkins May 24, 2017

Can one of the admins verify this patch?

mlpack-jenkins commented May 24, 2017

Can one of the admins verify this patch?

@17minutes

This comment has been minimized.

Show comment
Hide comment
@17minutes

17minutes Jun 5, 2017

Contributor

Wow, it was unreasonably buggy coding from my side, but it looks like I've finally got it right :)

So now, if I correctly remember the schedule, all it takes for Week 1 is to implement the data generator for SortTask and AddTask. I hope this won't be ridiculously hard, but it still requires to overload the evaluation methods for supporting sequences of binary numbers - in contrast to sequences of bits.

Contributor

17minutes commented Jun 5, 2017

Wow, it was unreasonably buggy coding from my side, but it looks like I've finally got it right :)

So now, if I correctly remember the schedule, all it takes for Week 1 is to implement the data generator for SortTask and AddTask. I hope this won't be ridiculously hard, but it still requires to overload the evaluation methods for supporting sequences of binary numbers - in contrast to sequences of bits.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jun 5, 2017

Member

I hope this won't be ridiculously hard, but it still requires to overload the evaluation methods for supporting sequences of binary numbers - in contrast to sequences of bits.

Not sure what evaluation method you mean here, can you explain further?

Member

zoq commented Jun 5, 2017

I hope this won't be ridiculously hard, but it still requires to overload the evaluation methods for supporting sequences of binary numbers - in contrast to sequences of bits.

Not sure what evaluation method you mean here, can you explain further?

@17minutes

This comment has been minimized.

Show comment
Hide comment
@17minutes

17minutes Jun 5, 2017

Contributor

Not sure what evaluation method you mean here, can you explain further?

I mean that even though other tasks are still evaluated by sequence precision, AddTask and SortTask require processing/outputting of sequences of numbers (ain't interesting to sort 0s and 1s 😃), in contrast to CopyTask, which requires to copy sequences of 0s and 1s. The current implementation of SequencePrecision takes two field<irowvec>s as arguments --- to support two other tasks, it needs to be able to take two field<imat>s as arguments.

Contributor

17minutes commented Jun 5, 2017

Not sure what evaluation method you mean here, can you explain further?

I mean that even though other tasks are still evaluated by sequence precision, AddTask and SortTask require processing/outputting of sequences of numbers (ain't interesting to sort 0s and 1s 😃), in contrast to CopyTask, which requires to copy sequences of 0s and 1s. The current implementation of SequencePrecision takes two field<irowvec>s as arguments --- to support two other tasks, it needs to be able to take two field<imat>s as arguments.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jun 5, 2017

Member

Thanks for the clarification, now I get what you mean 👍

In a previous comment you said you are currently at an internship, does the internship end this week or next week or something else? Just wanted to make sure you are able to work full-time on the project.

Member

zoq commented Jun 5, 2017

Thanks for the clarification, now I get what you mean 👍

In a previous comment you said you are currently at an internship, does the internship end this week or next week or something else? Just wanted to make sure you are able to work full-time on the project.

@17minutes

This comment has been minimized.

Show comment
Hide comment
@17minutes

17minutes Jun 5, 2017

Contributor

In a previous comment you said you are currently at an internship, does the internship end this week or next week or something else? Just wanted to make sure you are able to work full-time on the project.

Yes, both the internship and the entrance exam to Yandex School of Data Analysis have recently wrapped up (exam, the last event so far, was held on Sunday). So from here, I am able to work full-time.

Contributor

17minutes commented Jun 5, 2017

In a previous comment you said you are currently at an internship, does the internship end this week or next week or something else? Just wanted to make sure you are able to work full-time on the project.

Yes, both the internship and the entrance exam to Yandex School of Data Analysis have recently wrapped up (exam, the last event so far, was held on Sunday). So from here, I am able to work full-time.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jun 5, 2017

Member

Thanks again for the clarification, the course looks interesting: https://yandexdataschool.com/edu-process/program/data-analysis not sure which one you did, but I guess every course is somehow interesting, at least the description looks promising. Hopefully, your exam went well.

Just a reminder, would be great to see the first weekly report today or tomorrow.

Member

zoq commented Jun 5, 2017

Thanks again for the clarification, the course looks interesting: https://yandexdataschool.com/edu-process/program/data-analysis not sure which one you did, but I guess every course is somehow interesting, at least the description looks promising. Hopefully, your exam went well.

Just a reminder, would be great to see the first weekly report today or tomorrow.

@17minutes

This comment has been minimized.

Show comment
Hide comment
@17minutes

17minutes Jun 6, 2017

Contributor

Fixed the style issues you've mentioned in the review, now switching to the other two tasks. By the way, what's up with Jenkins? It's being much more severe that it used to be.

Thanks again for the clarification, the course looks interesting: https://yandexdataschool.com/edu-process/program/data-analysis not sure which one you did, but I guess every course is somehow interesting, at least the description looks promising. Hopefully, your exam went well.

That (Data Analysis) is precisely the program I've applied to :) However, I'm also going to take on some courses from Big Data program (e.g., parallel computation course - when you know that LAPACK is supreme to just writing for-loop, but don't know precisely why 😸)

Contributor

17minutes commented Jun 6, 2017

Fixed the style issues you've mentioned in the review, now switching to the other two tasks. By the way, what's up with Jenkins? It's being much more severe that it used to be.

Thanks again for the clarification, the course looks interesting: https://yandexdataschool.com/edu-process/program/data-analysis not sure which one you did, but I guess every course is somehow interesting, at least the description looks promising. Hopefully, your exam went well.

That (Data Analysis) is precisely the program I've applied to :) However, I'm also going to take on some courses from Big Data program (e.g., parallel computation course - when you know that LAPACK is supreme to just writing for-loop, but don't know precisely why 😸)

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jun 6, 2017

Member

The Jenkins Style Checks only checked files in mlpack/core now it also checks files in methods and tests. I fixed almost all issues that are not related to any PR here: #1019, so realistically you should only look at the files you changed and try to fix the issues pointed out by cpplint.

That (Data Analysis) is precisely the program I've applied to :) However, I'm also going to take on some courses from Big Data program (e.g., parallel computation course - when you know that LAPACK is supreme to just writing for-loop, but don't know precisely why 😸)

Right, I think in mlpack's context switching OpenBLAS with MKL would also result in some noticeable performance improvements at least in some situations.

Member

zoq commented Jun 6, 2017

The Jenkins Style Checks only checked files in mlpack/core now it also checks files in methods and tests. I fixed almost all issues that are not related to any PR here: #1019, so realistically you should only look at the files you changed and try to fix the issues pointed out by cpplint.

That (Data Analysis) is precisely the program I've applied to :) However, I'm also going to take on some courses from Big Data program (e.g., parallel computation course - when you know that LAPACK is supreme to just writing for-loop, but don't know precisely why 😸)

Right, I think in mlpack's context switching OpenBLAS with MKL would also result in some noticeable performance improvements at least in some situations.

@17minutes

This comment has been minimized.

Show comment
Hide comment
@17minutes

17minutes Jun 6, 2017

Contributor

Travis CI has also got crazy - now VanillaNetworkTest from convolutional_network_test.cpp also crashes with getting 50% classification error.

Contributor

17minutes commented Jun 6, 2017

Travis CI has also got crazy - now VanillaNetworkTest from convolutional_network_test.cpp also crashes with getting 50% classification error.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jun 6, 2017

Member

Sometimes test fail because they e.g. didn't reach the expected error in the specified number of iterations using the given starting point, for more information take a look at #922

Realistically, you don't have to worry about failing test that are not related with your code.

Member

zoq commented Jun 6, 2017

Sometimes test fail because they e.g. didn't reach the expected error in the specified number of iterations using the given starting point, for more information take a look at #922

Realistically, you don't have to worry about failing test that are not related with your code.

@17minutes

This comment has been minimized.

Show comment
Hide comment
@17minutes

17minutes Jun 6, 2017

Contributor

Now I have a problem I can't easily solve. I need to generate a random N*5 matrix (N 5-bit numbers). However, when I use randi(N, 5, distr_param(0, 1)), it returns a sequence of 00000 and 11111 (in other words, only rows get randomized - not columns).

I also tried setting manually all values to numbers from RNG (the current implementation), but it has the same problem. How to fix it?

Contributor

17minutes commented Jun 6, 2017

Now I have a problem I can't easily solve. I need to generate a random N*5 matrix (N 5-bit numbers). However, when I use randi(N, 5, distr_param(0, 1)), it returns a sequence of 00000 and 11111 (in other words, only rows get randomized - not columns).

I also tried setting manually all values to numbers from RNG (the current implementation), but it has the same problem. How to fix it?

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jun 6, 2017

Member

using:

arma::Mat<size_t> output = arma::randi<arma::Mat<size_t>>(10, 5, arma::distr_param(0, 1));

I get:

        0        0        0        1        0
        0        1        1        1        1
        1        1        1        0        1
        0        0        1        0        1
        1        0        1        1        1
        0        1        1        0        0
        0        1        0        1        1
        1        0        1        1        1
        1        0        0        1        0
        1        0        1        0        0

Isn't that what you expected?

Member

zoq commented Jun 6, 2017

using:

arma::Mat<size_t> output = arma::randi<arma::Mat<size_t>>(10, 5, arma::distr_param(0, 1));

I get:

        0        0        0        1        0
        0        1        1        1        1
        1        1        1        0        1
        0        0        1        0        1
        1        0        1        1        1
        0        1        1        0        0
        0        1        0        1        1
        1        0        1        1        1
        1        0        0        1        0
        1        0        1        0        0

Isn't that what you expected?

@17minutes

This comment has been minimized.

Show comment
Hide comment
@17minutes

17minutes Jun 6, 2017

Contributor

Isn't that what you expected?

Yes, but this one didn't work either :( Speaking of this, can I use vector<vector<int>> to create imat? So far I can't find any other viable way to fix that, but tomorrow I'll try again to debug it.

Speaking about the weekly report, I think it would be better to end tomorrow third task + fix the mentioned bug and then (once again, tomorrow) write the complete report on Week 1. What do you think?

Contributor

17minutes commented Jun 6, 2017

Isn't that what you expected?

Yes, but this one didn't work either :( Speaking of this, can I use vector<vector<int>> to create imat? So far I can't find any other viable way to fix that, but tomorrow I'll try again to debug it.

Speaking about the weekly report, I think it would be better to end tomorrow third task + fix the mentioned bug and then (once again, tomorrow) write the complete report on Week 1. What do you think?

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jun 6, 2017

Member

So, do you mean my output is what you like, but it's not working for you? If that's the case, can you reduce the problem to a simple case? Easier to debug the problem.

Sending the weekly report tomorrow, sounds fine for me.

Member

zoq commented Jun 6, 2017

So, do you mean my output is what you like, but it's not working for you? If that's the case, can you reduce the problem to a simple case? Easier to debug the problem.

Sending the weekly report tomorrow, sounds fine for me.

@17minutes

This comment has been minimized.

Show comment
Hide comment
@17minutes

17minutes Jun 7, 2017

Contributor

So, do you mean my output is what you like, but it's not working for you? If that's the case, can you reduce the problem to a simple case? Easier to debug the problem.

Yes, I was trying to achieve your output. I have resolved the problem - it was due to my really stupid bug in debug output producing.

I have also implemented AddTask, so now I have only one question - how to merge the style fixes from master branch to my fork?

After that, I'm planning to write the Week 1 report. Then (according to our schedule) we are switching to some real (but not augmented) models - I hope to implement LSTM baseline and record its scores.

Contributor

17minutes commented Jun 7, 2017

So, do you mean my output is what you like, but it's not working for you? If that's the case, can you reduce the problem to a simple case? Easier to debug the problem.

Yes, I was trying to achieve your output. I have resolved the problem - it was due to my really stupid bug in debug output producing.

I have also implemented AddTask, so now I have only one question - how to merge the style fixes from master branch to my fork?

After that, I'm planning to write the Week 1 report. Then (according to our schedule) we are switching to some real (but not augmented) models - I hope to implement LSTM baseline and record its scores.

@17minutes

This comment has been minimized.

Show comment
Hide comment
@17minutes

17minutes Jun 7, 2017

Contributor

Whoops, my bad, there were only my errors. Trying to fix them.

Contributor

17minutes commented Jun 7, 2017

Whoops, my bad, there were only my errors. Trying to fix them.

@17minutes

This comment has been minimized.

Show comment
Hide comment
@17minutes

17minutes Jun 7, 2017

Contributor

Finally a clear style-check run :)

Contributor

17minutes commented Jun 7, 2017

Finally a clear style-check run :)

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jun 7, 2017

Member

After that, I'm planning to write the Week 1 report. Then (according to our schedule) we are switching to some real (but not augmented) models - I hope to implement LSTM baseline and record its scores.

Sounds good, I think we could also compare against GRU (once it's merged), without changing a lot of code.

Member

zoq commented Jun 7, 2017

After that, I'm planning to write the Week 1 report. Then (according to our schedule) we are switching to some real (but not augmented) models - I hope to implement LSTM baseline and record its scores.

Sounds good, I think we could also compare against GRU (once it's merged), without changing a lot of code.

@17minutes

This comment has been minimized.

Show comment
Hide comment
@17minutes

17minutes Jun 7, 2017

Contributor

AppVeyor is running for a really long time. How can I dequeue my previous commits from here: https://ci.appveyor.com/project/mlpack/mlpack/history

UPDATE They have just been dequeued. Thanks ;)

Contributor

17minutes commented Jun 7, 2017

AppVeyor is running for a really long time. How can I dequeue my previous commits from here: https://ci.appveyor.com/project/mlpack/mlpack/history

UPDATE They have just been dequeued. Thanks ;)

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jun 7, 2017

Member

I'm not sure if you are allowed to cancel your job at the moment, you could login into appveyor and see if you do, but in the meantime let me do it for you.

Member

zoq commented Jun 7, 2017

I'm not sure if you are allowed to cancel your job at the moment, you could login into appveyor and see if you do, but in the meantime let me do it for you.

Show outdated Hide outdated src/mlpack/methods/ann/augmented/tasks/copy.hpp
Show outdated Hide outdated src/mlpack/methods/ann/augmented/tasks/copy.hpp
Show outdated Hide outdated src/mlpack/methods/ann/augmented/tasks/copy.hpp
Show outdated Hide outdated src/mlpack/methods/ann/augmented/tasks/copy_impl.hpp
*
* @param input The variable to store input sequences.
* @param labels The variable to store output sequences.
* @param batchSize The dataset size.

This comment has been minimized.

@zoq

zoq Jun 8, 2017

Member

Perhaps batchSize isn't the best parameter name here? Do you think size could work?

@zoq

zoq Jun 8, 2017

Member

Perhaps batchSize isn't the best parameter name here? Do you think size could work?

This comment has been minimized.

@17minutes

17minutes Jun 10, 2017

Contributor

I would like to leave the long name, because with size variable would be not as clear which size is meant (sample size? maximum sequence length? size of sequence elements?). The long name, in contrast, eliminates any chance to slip later.

@17minutes

17minutes Jun 10, 2017

Contributor

I would like to leave the long name, because with size variable would be not as clear which size is meant (sample size? maximum sequence length? size of sequence elements?). The long name, in contrast, eliminates any chance to slip later.

This comment has been minimized.

@zoq

zoq Jun 10, 2017

Member

Okay, sounds reasonable to me.

@zoq

zoq Jun 10, 2017

Member

Okay, sounds reasonable to me.

Show outdated Hide outdated src/mlpack/methods/ann/augmented/tasks/copy_impl.hpp
Show outdated Hide outdated src/mlpack/methods/ann/augmented/tasks/copy_impl.hpp
Show outdated Hide outdated src/mlpack/methods/ann/augmented/tasks/copy_impl.hpp
Show outdated Hide outdated src/mlpack/methods/ann/augmented/tasks/copy_impl.hpp
@17minutes

This comment has been minimized.

Show comment
Hide comment
@17minutes

17minutes Jun 13, 2017

Contributor

I keep trying to create an LSTM baseline :) Right now, it kinda works, but there are two big problems:

  • I have no idea hwo to change model's rho (for LSTM layer, I just set its rho to the maximum required value) For this reason, LSTM model just kills sequences of length 2, but is unable to emit third symbol.
  • There is some weird bug when setting nRepeats to some value > 1. Precisely, the model.Train() instruction crashes. I wonder what's the difference between these cases.

Can you help me resolve those issues, please?

The current unit test code is in the last commit.

Contributor

17minutes commented Jun 13, 2017

I keep trying to create an LSTM baseline :) Right now, it kinda works, but there are two big problems:

  • I have no idea hwo to change model's rho (for LSTM layer, I just set its rho to the maximum required value) For this reason, LSTM model just kills sequences of length 2, but is unable to emit third symbol.
  • There is some weird bug when setting nRepeats to some value > 1. Precisely, the model.Train() instruction crashes. I wonder what's the difference between these cases.

Can you help me resolve those issues, please?

The current unit test code is in the last commit.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jun 13, 2017

Member

I looked into the issue and setting the correct sequence length and it turns out we have to do a little bit more as setting the rho value since the input size is only set once (https://github.com/mlpack/mlpack/blob/master/src/mlpack/methods/ann/rnn_impl.hpp#L206). Also, I think it would be convenient if the model propagates the rho value through the model, that way we only have to change it once. Meaning we need another routine that does this for us.

Member

zoq commented Jun 13, 2017

I looked into the issue and setting the correct sequence length and it turns out we have to do a little bit more as setting the rho value since the input size is only set once (https://github.com/mlpack/mlpack/blob/master/src/mlpack/methods/ann/rnn_impl.hpp#L206). Also, I think it would be convenient if the model propagates the rho value through the model, that way we only have to change it once. Meaning we need another routine that does this for us.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jun 13, 2017

Member

Here is a quick patch

https://gist.github.com/zoq/24f8b2e4826d837d604f9613615763bb
https://gist.github.com/zoq/01952c2be67bd9fbeeeac5d6c69b39cd
https://gist.github.com/zoq/c7ffc1f16e654c94097fd79d1687fef3

Let me know if I should push the changes to my fork if that's easier to read for you.

and here is my output for nRepeat = 2

Input:
        0   1.0000   1.0000

Model output:
        0   1.0000   1.0000

True output:
        0   1.0000   1.0000

Input:
   1.0000        0   1.0000

Model output:
   1.0000        0   1.0000

True output:
   1.0000        0   1.0000

=======================================
Input:
   1.0000   1.0000

Model output:
   1.0000   1.0000

True output:
   1.0000   1.0000

=======================================
Input:
        0        0

Model output:
        0        0

True output:
        0        0

=======================================
Final score: 1

and the output for nRepeat = 3

Input:
        0   1.0000   1.0000

Model output:
        0        0        0

True output:
        0   1.0000   1.0000        0   1.0000   1.0000

=======================================
Input:
   1.0000   1.0000   1.0000

Model output:
   1.0000   1.0000   1.0000

True output:
   1.0000   1.0000   1.0000   1.0000   1.0000   1.0000

=======================================
Final score: 0

As for the repeat issue, without any further information, I can't see how the model should predict the right output.

Member

zoq commented Jun 13, 2017

Here is a quick patch

https://gist.github.com/zoq/24f8b2e4826d837d604f9613615763bb
https://gist.github.com/zoq/01952c2be67bd9fbeeeac5d6c69b39cd
https://gist.github.com/zoq/c7ffc1f16e654c94097fd79d1687fef3

Let me know if I should push the changes to my fork if that's easier to read for you.

and here is my output for nRepeat = 2

Input:
        0   1.0000   1.0000

Model output:
        0   1.0000   1.0000

True output:
        0   1.0000   1.0000

Input:
   1.0000        0   1.0000

Model output:
   1.0000        0   1.0000

True output:
   1.0000        0   1.0000

=======================================
Input:
   1.0000   1.0000

Model output:
   1.0000   1.0000

True output:
   1.0000   1.0000

=======================================
Input:
        0        0

Model output:
        0        0

True output:
        0        0

=======================================
Final score: 1

and the output for nRepeat = 3

Input:
        0   1.0000   1.0000

Model output:
        0        0        0

True output:
        0   1.0000   1.0000        0   1.0000   1.0000

=======================================
Input:
   1.0000   1.0000   1.0000

Model output:
   1.0000   1.0000   1.0000

True output:
   1.0000   1.0000   1.0000   1.0000   1.0000   1.0000

=======================================
Final score: 0

As for the repeat issue, without any further information, I can't see how the model should predict the right output.

@17minutes

This comment has been minimized.

Show comment
Hide comment
@17minutes

17minutes Jun 14, 2017

Contributor

Implemented the changed you described, now it works nicely for nRepeats=1. Interstingly, that on my machine nRepeats=3 crashes (but your output suggests that the model has learned to copy but failed to learn to repeat) with the same message:

error: subtraction: incompatible matrix dimensions: 1x1 and 2x1
unknown location(0): fatal error: in "AugmentedRNNsTasks/LSTMBaselineTest": std::logic_error: subtraction: incompatible matrix dimensions: 1x1 and 2x1

Right now I'm implementing similar test for AddTask and SortTask, after what I'll proceed to record the scores and write Week 2 report.

However, I would like to know whether unit test is the appropriate place for running a baseline solution. Maybe I should implement it somewhere else, e.g. some kind of examples binary?

Contributor

17minutes commented Jun 14, 2017

Implemented the changed you described, now it works nicely for nRepeats=1. Interstingly, that on my machine nRepeats=3 crashes (but your output suggests that the model has learned to copy but failed to learn to repeat) with the same message:

error: subtraction: incompatible matrix dimensions: 1x1 and 2x1
unknown location(0): fatal error: in "AugmentedRNNsTasks/LSTMBaselineTest": std::logic_error: subtraction: incompatible matrix dimensions: 1x1 and 2x1

Right now I'm implementing similar test for AddTask and SortTask, after what I'll proceed to record the scores and write Week 2 report.

However, I would like to know whether unit test is the appropriate place for running a baseline solution. Maybe I should implement it somewhere else, e.g. some kind of examples binary?

@@ -33,7 +33,6 @@
#include <mlpack/methods/ann/layer/parametric_relu.hpp>
#include <mlpack/methods/ann/layer/reinforce_normal.hpp>
#include <mlpack/methods/ann/layer/select.hpp>
#include <mlpack/methods/ann/layer/cross_entropy_error.hpp>

This comment has been minimized.

@zoq

zoq Jul 31, 2017

Member

Not sure what happened here, but we should not remove the recently introduced cross entropy error function.

@zoq

zoq Jul 31, 2017

Member

Not sure what happened here, but we should not remove the recently introduced cross entropy error function.

@zoq

Mainly minor style issues.

Show outdated Hide outdated src/mlpack/core/math/random.hpp
Show outdated Hide outdated src/mlpack/core/math/random.hpp
Show outdated Hide outdated src/mlpack/core/math/random.hpp
Show outdated Hide outdated src/mlpack/methods/ann/augmented/tasks/add_impl.hpp
Show outdated Hide outdated src/mlpack/methods/ann/augmented/tasks/add_impl.hpp
Show outdated Hide outdated src/mlpack/tests/augmented_rnns_tasks_test.cpp
Show outdated Hide outdated src/mlpack/tests/augmented_rnns_tasks_test.cpp
Show outdated Hide outdated src/mlpack/tests/augmented_rnns_tasks_test.cpp
Show outdated Hide outdated src/mlpack/tests/augmented_rnns_tasks_test.cpp
BOOST_AUTO_TEST_SUITE(RandomTest);
// Test for RandInt() sampler from discrete uniform distribution.
BOOST_AUTO_TEST_CASE(DiscreteUniformRandomTest)

This comment has been minimized.

@zoq

zoq Jul 31, 2017

Member

Thanks for write a test for the method!

@zoq

zoq Jul 31, 2017

Member

Thanks for write a test for the method!

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Aug 1, 2017

Member

Temporary fixed the disk space issue, can you go through the issues listed here: http://masterblaster.mlpack.org/job/pull-requests%20mlpack%20style%20checks/656/cppcheckResult/source.all/?before=5&after=5

Member

zoq commented Aug 1, 2017

Temporary fixed the disk space issue, can you go through the issues listed here: http://masterblaster.mlpack.org/job/pull-requests%20mlpack%20style%20checks/656/cppcheckResult/source.all/?before=5&after=5

@zoq

zoq approved these changes Aug 2, 2017

I think it looks good and we should merge it. We should let this sit for 3 days, before merge to give anyone else time to comment.

@rcurtin

rcurtin approved these changes Aug 2, 2017

Thanks so much for the hard work with this PR. I agree that it is ready, if you can take care of the comment about RandInt().

Show outdated Hide outdated src/mlpack/core/math/random.hpp
Show outdated Hide outdated src/mlpack/methods/ann/augmented/tasks/copy_impl.hpp
input(i).rows(origPtr, origPtr + bitLen - 1);
ptr += bitLen;
origPtr += bitLen;
sepInput.at(ptr, 0) = 0.5;

This comment has been minimized.

@rcurtin

rcurtin Aug 2, 2017

Member

Ok, I see---thanks for the clarification.

@rcurtin

rcurtin Aug 2, 2017

Member

Ok, I see---thanks for the clarification.

@rcurtin

rcurtin approved these changes Aug 5, 2017

Thanks for the refactoring. I'm glad we can keep the tests too. :) I only have a couple minor comments that you can address if you like (it'll make the code just a bit cleaner), otherwise I agree this is ready for merge.

{
arma::vec weights(maxLength - 1);
mlpack::distribution::DiscreteDistribution d(1);

This comment has been minimized.

@zoq

zoq Aug 6, 2017

Member

We have to include #include <mlpack/core/dists/discrete_distribution.hpp> to avoid an undefined method issue. The same for the Add task.

@zoq

zoq Aug 6, 2017

Member

We have to include #include <mlpack/core/dists/discrete_distribution.hpp> to avoid an undefined method issue. The same for the Add task.

@zoq zoq merged commit 94907ce into mlpack:master Aug 7, 2017

3 checks passed

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 Aug 7, 2017

Member

Thanks for the great contribution!

Member

zoq commented Aug 7, 2017

Thanks for the great contribution!

@zoq zoq referenced this pull request Aug 8, 2017

Merged

GRU layer for ANN module #1018

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