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

Single output for multiple timestep input for RNN #1348

Merged
merged 11 commits into from Apr 20, 2018

Conversation

Projects
None yet
4 participants
@scrcro
Contributor

scrcro commented Mar 31, 2018

Add support for multiple timestep -- one output for RNN

@scrcro scrcro changed the title from Rnnmultitimestep to Single output for multiple timestep input for RNN Mar 31, 2018

@scrcro

This comment has been minimized.

Contributor

scrcro commented Apr 2, 2018

Hello
I made the changes for supporting multiple timestep input and one output.
The style check failed on a file that I did not change. Hence I think can be ignored.
The Linux build failed, but looks like some env issue. Please suggest.
-scrcro

@scrcro

This comment has been minimized.

Contributor

scrcro commented Apr 2, 2018

I am done with the changes. It is ready for review.

@rcurtin

Thanks for the contribution. I think the changes are pretty simple and I'm happy to see it was this easy. I do think we should be sure to clarify the documentation, so if you are able to do that it would be great.

I have just a few comments about the code itself; let me know if I can clarify any of them. In any case we should definitely also wait on Marcus's input here before merging, since he may have been envisioning a different solution (I am not sure).

uint64_t timeSeed =
std::chrono::high_resolution_clock::now().time_since_epoch().count();
std::seed_seq ss{uint32_t(timeSeed & 0xffffffff), uint32_t(timeSeed >> 32)};
rng.seed(ss);

This comment has been minimized.

@rcurtin

rcurtin Apr 2, 2018

Member

Can you use the utilities in src/mlpack/core/math/random.hpp for this instead?

This comment has been minimized.

@scrcro

scrcro Apr 3, 2018

Contributor

Done

@@ -15,6 +15,10 @@
#include <mlpack/methods/ann/layer/layer.hpp>
#include <mlpack/methods/ann/rnn.hpp>
#include <mlpack/core/data/binarize.hpp>
#include <mlpack/core/optimizers/rmsprop/rmsprop.hpp>
#include <cmath>

This comment has been minimized.

@rcurtin

rcurtin Apr 2, 2018

Member

This should already be included by prereqs.hpp so I think there is no need to include it here.

This comment has been minimized.

@scrcro

scrcro Apr 3, 2018

Contributor

RMSProp is not included somehow, I was getting a compile error. Therefore I had to add it.
Removed cmath

@@ -201,14 +201,19 @@ double RNN<OutputLayerType, InitializationRuleType, CustomLayers...>::Evaluate(
ResetCells();
double performance = 0;
size_t respSeq = 0;

This comment has been minimized.

@rcurtin

rcurtin Apr 2, 2018

Member

Do you think it would be useful to add a simple check on the shape of the responses if single is true or false? Basically if single == true, we should ensure that the responses only have one slice. And I think we should also be sure to point this out in the documentation.

This comment has been minimized.

@scrcro

scrcro Apr 3, 2018

Contributor

I need to go through the code for the output to do this. Let me check.

* @param normalize Whether to normalise the data. This may be required for some
* layers like LSTM. Default is true.
*/
void GenerateNoisySinRNN(arma::cube& data, arma::cube& labels, size_t rho,

This comment has been minimized.

@rcurtin

rcurtin Apr 2, 2018

Member

I think maybe we should rename this---it's not creating an RNN, it's creating data, so I think the method name should reflect that.

This comment has been minimized.

@scrcro

scrcro Apr 3, 2018

Contributor

Well you are right on the data. At the end though I rearrange the data to make two cubes of input data and labels so that they can be used in the RNN training directly. Hence the name. I changed the documentation on the code also. Let me think of a better name..

@scrcro

This comment has been minimized.

Contributor

scrcro commented Apr 3, 2018

I also noticed something strange. When I run my test in isolation the mean error I get is lot less. But when I run my test with the other test, it seems to generate error which is much higher. Please see below:

hduser_@ubuntu:~/git/mlpack/build$ bin/mlpack_test -t RecurrentNetworkTest
Running 13 test cases...
/home/hduser_/git/mlpack/src/mlpack/tests/recurrent_network_test.cpp(1226): fatal error: in "RecurrentNetworkTest/MultiTimestepTest": critical check err <= 1e-04 has failed [0.0025246568949844257 > 0.0001]

*** 1 failure is detected in the test module "mlpackTest"
hduser_@ubuntu:~/git/mlpack/build$ bin/mlpack_test -t RecurrentNetworkTest/MultiTimestepTest
Running 1 test case...

*** No errors detected

Is there a reason that the test should be affected by other tests?

@Manthan-R-Sheth

This comment has been minimized.

Contributor

Manthan-R-Sheth commented Apr 3, 2018

@scrcro
I think one possible reason could be the random seed used in the test. Both the executions will have different random seeds used for the generation of the data and labels.

Review comments incorporated.
Removed some unwanted code.
@rcurtin

This comment has been minimized.

Member

rcurtin commented Apr 3, 2018

Right, I think Manthan is correct here. It would probably be good to run the test many times with different random seeds and ensure that it does not fail. You may have to adjust the tolerance of the test.

@scrcro

This comment has been minimized.

Contributor

scrcro commented Apr 3, 2018

@@ -201,15 +201,18 @@ double RNN<OutputLayerType, InitializationRuleType, CustomLayers...>::Evaluate(
ResetCells();
double performance = 0;
size_t respSeq = 0;

This comment has been minimized.

@zoq

zoq Apr 3, 2018

Member

Do you mind to use a more descriptive name for the parameter e.g. responsesSeq?

int i = 0;
double interval = numCycles / freq / points;
RandomSeed(20);

This comment has been minimized.

@zoq

zoq Apr 3, 2018

Member

Not sure I get the idea here, why do we use a static random seed? Ideally this should work with every seed.

* layers like LSTM. Default is true.
*/
void GenerateNoisySinRNN(arma::cube& data, arma::cube& labels, size_t rho,
size_t outputSteps = 1, const int dataPoints = 100,

This comment has been minimized.

@zoq

zoq Apr 3, 2018

Member

Can you use a single line for each parameter? See https://github.com/mlpack/mlpack/wiki/DesignGuidelines#method-declarations for more information, I think right now this isn't easy to read.

{
points += rho - r + outputSteps;
}
arma::colvec x(points);

This comment has been minimized.

@zoq

zoq Apr 3, 2018

Member

Another picky style comment; can you remove the extra spaces?

scrcro added some commits Apr 4, 2018

Changed the error calculation to reflect the error more accurately. A…
…lso reduced the test data for tests to complete faster.
@scrcro

This comment has been minimized.

Contributor

scrcro commented Apr 7, 2018

I am done with the changes as suggested. I also made some adjustments to the tests to make them execute faster and report the errors more accurately.

@scrcro

This comment has been minimized.

Contributor

scrcro commented Apr 9, 2018

Please review. The build was successful.

@scrcro

This comment has been minimized.

Contributor

scrcro commented Apr 13, 2018

Hello
Do you think this can be merged now? If there are other review comments please let me know.
cheers

@@ -283,6 +286,14 @@ void RNN<OutputLayerType, InitializationRuleType, CustomLayers...>::Gradient(
{
error.zeros();
}
else if (single && seqNum == 0)
{
outputLayer.Backward(std::move(boost::apply_visitor(

This comment has been minimized.

@zoq

zoq Apr 13, 2018

Member

Picky style comment; When wrapping a line, the next line should be tabbed twice from where the previous line began. See https://github.com/mlpack/mlpack/wiki/DesignGuidelines#line-length-and-wrapping for more information.

double interval = numCycles / freq / points;
uint64_t timeSeed =
std::chrono::high_resolution_clock::now().time_since_epoch().count();
RandomSeed(timeSeed);

This comment has been minimized.

@zoq

zoq Apr 13, 2018

Member

I don't think we should force a random seed here, this would avoid the possibility to set a static seed e.g. for debugging.

This comment has been minimized.

@scrcro

scrcro Apr 13, 2018

Contributor

I am not very familiar with the random number generation in mlpack. Do you want me to remove the RandomSeed call altogether?

This comment has been minimized.

@zoq

zoq Apr 13, 2018

Member

Yeah, that would solve the issue.

const double phase = 0,
const int noisePercent = 20,
const double numCycles = 6.0,
const bool normalize = true)

This comment has been minimized.

@zoq

zoq Apr 13, 2018

Member

Another picky style comment, can you remove the extra spaces?

* @param rho The size of the sequence of each data point
* @param outputSteps How many output steps to consider for every rho inputs
* @param dataPoints The number of generated data points. The actual generated
* data points may be more than this to adjust to the outputSteps.

This comment has been minimized.

@zoq

zoq Apr 13, 2018

Member

See the comment above.

y = arma::normalise(x);
// Now break this into columns of rho size slices.
size_t n_columns = y.n_elem / rho;

This comment has been minimized.

@zoq

zoq Apr 13, 2018

Member

Can you use camel casing for all names, see https://github.com/mlpack/mlpack/wiki/DesignGuidelines#naming-conventions for more information.

@scrcro

This comment has been minimized.

Contributor

scrcro commented Apr 14, 2018

Hi
Review comments addressed. Please have a look.

@zoq

zoq approved these changes Apr 16, 2018

Thanks for the contribution; I'll go ahead and merge this in 3 days to leave time for any other comments. I'll fix the remaining style issue once this is merged.

@zoq zoq merged commit 0b4905b into mlpack:master Apr 20, 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.

Member

zoq commented Apr 20, 2018

Thanks again for a great contribution.

@scrcro

This comment has been minimized.

Contributor

scrcro commented Apr 21, 2018

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