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

Conventional Neural Evolution - Optimizer #1088

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@kartik-nighania
Contributor

kartik-nighania commented Aug 9, 2017

Works as an optimizer for vanilla network and converges using probability and evolution strategy

@kartik-nighania

This comment has been minimized.

Show comment
Hide comment
@kartik-nighania

kartik-nighania Aug 11, 2017

Contributor

Hello @zoq
Can u look at the PR and suggest improvements and style fixes..

The test case is under evaluation
After this a tutorial is tobe made

Contributor

kartik-nighania commented Aug 11, 2017

Hello @zoq
Can u look at the PR and suggest improvements and style fixes..

The test case is under evaluation
After this a tutorial is tobe made

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Aug 12, 2017

Member

Testing it on XOR sounds good to me; about the issue, you use two nodes as output, but your label is just a single node. I would use 0, 1 as the label and only a single output node:

FFN<NegativeLogLikelihood<> > model;
model.Add<Linear<> >(t2, 2);
model.Add<SigmoidLayer<> >();
model.Add<Linear<> >(2, 1);
model.Add<LogSoftMax<> >();
Member

zoq commented Aug 12, 2017

Testing it on XOR sounds good to me; about the issue, you use two nodes as output, but your label is just a single node. I would use 0, 1 as the label and only a single output node:

FFN<NegativeLogLikelihood<> > model;
model.Add<Linear<> >(t2, 2);
model.Add<SigmoidLayer<> >();
model.Add<Linear<> >(2, 1);
model.Add<LogSoftMax<> >();
@kartik-nighania

This comment has been minimized.

Show comment
Hide comment
@kartik-nighania

kartik-nighania Aug 12, 2017

Contributor

@zoq
in the newly test case added

error: Mat::operator(): index out of bounds
terminate called after throwing an instance of 'std::logic_error'
  what():  Mat::operator(): index out of bounds
Aborted (core dumped)

why am i getting this error. Need help

Contributor

kartik-nighania commented Aug 12, 2017

@zoq
in the newly test case added

error: Mat::operator(): index out of bounds
terminate called after throwing an instance of 'std::logic_error'
  what():  Mat::operator(): index out of bounds
Aborted (core dumped)

why am i getting this error. Need help

@kartik-nighania

This comment has been minimized.

Show comment
Hide comment
@kartik-nighania

kartik-nighania Aug 12, 2017

Contributor

i get the same error using StandardSGD also

Contributor

kartik-nighania commented Aug 12, 2017

i get the same error using StandardSGD also

@kartik-nighania

This comment has been minimized.

Show comment
Hide comment
@kartik-nighania

kartik-nighania Aug 13, 2017

Contributor

@zoq
please suggest me improvements for the tutorial doc added.
thanks

Contributor

kartik-nighania commented Aug 13, 2017

@zoq
please suggest me improvements for the tutorial doc added.
thanks

Show outdated Hide outdated src/mlpack/core/optimizers/cne/cne.hpp
Show outdated Hide outdated src/mlpack/core/optimizers/cne/cne.hpp
Show outdated Hide outdated src/mlpack/core/optimizers/cne/cne.hpp
Show outdated Hide outdated src/mlpack/core/optimizers/cne/cne.hpp
Show outdated Hide outdated src/mlpack/core/optimizers/cne/cne.hpp
Show outdated Hide outdated src/mlpack/core/optimizers/cne/cne_impl.hpp
Show outdated Hide outdated src/mlpack/core/optimizers/cne/cne_impl.hpp
Show outdated Hide outdated src/mlpack/core/optimizers/cne/cne_impl.hpp
Show outdated Hide outdated src/mlpack/core/optimizers/cne/cne_impl.hpp
Show outdated Hide outdated src/mlpack/tests/cne_test.cpp
@kartik-nighania

This comment has been minimized.

Show comment
Hide comment
@kartik-nighania

kartik-nighania Aug 16, 2017

Contributor

@zoq

  • Test Iris dataset now works fine and the time is now down from timeout -> 247 sec -> 4.91 / 1.62 in both the machines.

  • some variables are taken out of reproduction to avoid multiple time same calculations

  • two false input checks have been added at the start of the constructor itself.

  • 2 minor code change remains, as suggested by u today

Thanks a lot for spending so much time for this in depth code review. It was very helpful

Contributor

kartik-nighania commented Aug 16, 2017

@zoq

  • Test Iris dataset now works fine and the time is now down from timeout -> 247 sec -> 4.91 / 1.62 in both the machines.

  • some variables are taken out of reproduction to avoid multiple time same calculations

  • two false input checks have been added at the start of the constructor itself.

  • 2 minor code change remains, as suggested by u today

Thanks a lot for spending so much time for this in depth code review. It was very helpful

@kartik-nighania

This comment has been minimized.

Show comment
Hide comment
@kartik-nighania

kartik-nighania Aug 18, 2017

Contributor

@zoq

//! Modify weights with some noise for the evolution of next generation.
void CNE::Mutate()
{
  arma::vec bestCandidate = population.col(index[0]);

  // Mutate the whole matrix with the given rate and probability.
  arma::mat delta = arma::randu<arma::mat>(size(population));
  delta.elem(arma::find(delta < mutationProb)).fill(mlpack::math::RandNormal(0, mutationSize));

  population += delta;
  population.col(index[0]) = bestCandidate;
}

.fill() takes a finite value, that is the problem.
I cant think of anything that takes both mutationProb and mutationSize into consideration otherwise the method becomes more time consuming than the iterative process itself.
what should i do?

Contributor

kartik-nighania commented Aug 18, 2017

@zoq

//! Modify weights with some noise for the evolution of next generation.
void CNE::Mutate()
{
  arma::vec bestCandidate = population.col(index[0]);

  // Mutate the whole matrix with the given rate and probability.
  arma::mat delta = arma::randu<arma::mat>(size(population));
  delta.elem(arma::find(delta < mutationProb)).fill(mlpack::math::RandNormal(0, mutationSize));

  population += delta;
  population.col(index[0]) = bestCandidate;
}

.fill() takes a finite value, that is the problem.
I cant think of anything that takes both mutationProb and mutationSize into consideration otherwise the method becomes more time consuming than the iterative process itself.
what should i do?

@kartik-nighania

This comment has been minimized.

Show comment
Hide comment
@kartik-nighania

kartik-nighania Aug 18, 2017

Contributor

@zoq
after the above suggested change, I think this PR is complete. Please let me know for further improvements.

Contributor

kartik-nighania commented Aug 18, 2017

@zoq
after the above suggested change, I think this PR is complete. Please let me know for further improvements.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Aug 18, 2017

Member

There is one thing that I missed, it's not granted that iterate is a single column matrix. So I propose to use arma::cube instead of arma::mat to store the population, so:

population = arma::randu(iterate.n_rows, populationSize);

becomes

population = arma::randu(iterate.n_rows, iterate.n_cols, populationSize);

here is the refactored Corossover function:

void CNE::Crossover(const size_t mom,
                    const size_t dad,
                    const size_t child1,
                    const size_t child2)
{
  // Replace the cadidates with parents at their place.
  population.slice(child1) = population.slice(mom);
  population.slice(child2) = population.slice(dad);

  double rand;

  // Preallocate random selection vector (values between 0 and 1).
  arma::vec selection = arma::randu(population.n_rows * population.n_cols);

  // Randomly alter mom and dad genome weights to get two different childs.
  for (size_t i = 0; i < population.n_elem; i++)
  {
    // Using it to alter the weights of the childs.
    if (selection(i) > 0.5)
    {
      population.slice(child1)(i) = population.slice(mom)(i);
      population.slice(child2)(i) = population.slice(dad)(i);
    }
    else
    {
      population.slice(child1)(i) = population.slice(dad)(i);
      population.slice(child2)(i) = population.slice(mom)(i);
    }
  }
}

About the Mutate I think we can write something like:

  double mutationProb = 0.5;
  double mutationSize = 0.02;

  // noise
  std::cout << arma::randu(5, 5) << std::endl;

  // noise < mutationProp
  std::cout << (arma::randu(5, 5) < mutationProb) << std::endl;

  // delta
  std::cout << mutationSize * arma::randn(5, 5) << std::endl;

  // complete update matrix
  std::cout << (arma::randu(5, 5) < mutationProb) % (mutationSize * arma::randn(5, 5)) << std::endl;

here is the output:

  // noise
   0.0002   0.0361   0.8205   0.1831   0.6022
   0.0254   0.0820   0.9466   0.4364   0.7958
   0.3789   0.6139   0.1122   0.1771   0.5465
   0.5490   0.6238   0.7952   0.8186   0.4000
   0.2536   0.4979   0.2296   0.5374   0.4618

  // noise < mutationProp
        1        0        0        0        0
        1        0        0        0        0
        1        1        1        0        1
        0        1        0        1        1
        1        0        1        0        1
  // delta
  -0.0070   0.0105   0.0165   0.0127  -0.0195
  -0.0398   0.0156  -0.0072   0.0012   0.0112
  -0.0107  -0.0040   0.0280   0.0252   0.0081
   0.0148   0.0249  -0.0201  -0.0005  -0.0198
  -0.0222   0.0085   0.0194  -0.0020   0.0025

  // complete update matrix
        0   0.0038   0.0457   0.0048   0.0024
   0.0212        0  -0.0203        0        0
        0        0  -0.0067        0  -0.0054
  -0.0004  -0.0026        0   0.0045        0
   0.0090        0        0  -0.0170        0

so Mutate becomes:

void CNE::Mutate()
{
  // Helper variables.
  double noise;
  double delta;

  // Mutate the whole matrix with the given rate and probability.
  // The best candidate is not altered.
  for (size_t i = 1; i < populationSize; i++)
  {
    population.slice(index(i)) += (arma::randu(
        population.n_rows, population.n_cols) < mutationProb) %
        (mutationSize * arma::randn(population.n_rows, population.n_cols));
  }
}

Let me know if I should clarify anything.

Member

zoq commented Aug 18, 2017

There is one thing that I missed, it's not granted that iterate is a single column matrix. So I propose to use arma::cube instead of arma::mat to store the population, so:

population = arma::randu(iterate.n_rows, populationSize);

becomes

population = arma::randu(iterate.n_rows, iterate.n_cols, populationSize);

here is the refactored Corossover function:

void CNE::Crossover(const size_t mom,
                    const size_t dad,
                    const size_t child1,
                    const size_t child2)
{
  // Replace the cadidates with parents at their place.
  population.slice(child1) = population.slice(mom);
  population.slice(child2) = population.slice(dad);

  double rand;

  // Preallocate random selection vector (values between 0 and 1).
  arma::vec selection = arma::randu(population.n_rows * population.n_cols);

  // Randomly alter mom and dad genome weights to get two different childs.
  for (size_t i = 0; i < population.n_elem; i++)
  {
    // Using it to alter the weights of the childs.
    if (selection(i) > 0.5)
    {
      population.slice(child1)(i) = population.slice(mom)(i);
      population.slice(child2)(i) = population.slice(dad)(i);
    }
    else
    {
      population.slice(child1)(i) = population.slice(dad)(i);
      population.slice(child2)(i) = population.slice(mom)(i);
    }
  }
}

About the Mutate I think we can write something like:

  double mutationProb = 0.5;
  double mutationSize = 0.02;

  // noise
  std::cout << arma::randu(5, 5) << std::endl;

  // noise < mutationProp
  std::cout << (arma::randu(5, 5) < mutationProb) << std::endl;

  // delta
  std::cout << mutationSize * arma::randn(5, 5) << std::endl;

  // complete update matrix
  std::cout << (arma::randu(5, 5) < mutationProb) % (mutationSize * arma::randn(5, 5)) << std::endl;

here is the output:

  // noise
   0.0002   0.0361   0.8205   0.1831   0.6022
   0.0254   0.0820   0.9466   0.4364   0.7958
   0.3789   0.6139   0.1122   0.1771   0.5465
   0.5490   0.6238   0.7952   0.8186   0.4000
   0.2536   0.4979   0.2296   0.5374   0.4618

  // noise < mutationProp
        1        0        0        0        0
        1        0        0        0        0
        1        1        1        0        1
        0        1        0        1        1
        1        0        1        0        1
  // delta
  -0.0070   0.0105   0.0165   0.0127  -0.0195
  -0.0398   0.0156  -0.0072   0.0012   0.0112
  -0.0107  -0.0040   0.0280   0.0252   0.0081
   0.0148   0.0249  -0.0201  -0.0005  -0.0198
  -0.0222   0.0085   0.0194  -0.0020   0.0025

  // complete update matrix
        0   0.0038   0.0457   0.0048   0.0024
   0.0212        0  -0.0203        0        0
        0        0  -0.0067        0  -0.0054
  -0.0004  -0.0026        0   0.0045        0
   0.0090        0        0  -0.0170        0

so Mutate becomes:

void CNE::Mutate()
{
  // Helper variables.
  double noise;
  double delta;

  // Mutate the whole matrix with the given rate and probability.
  // The best candidate is not altered.
  for (size_t i = 1; i < populationSize; i++)
  {
    population.slice(index(i)) += (arma::randu(
        population.n_rows, population.n_cols) < mutationProb) %
        (mutationSize * arma::randn(population.n_rows, population.n_cols));
  }
}

Let me know if I should clarify anything.

@kartik-nighania

This comment has been minimized.

Show comment
Hide comment
@kartik-nighania

kartik-nighania Aug 18, 2017

Contributor

@zoq
Ah.. i see. I was thinking of how to make sure less than mutationSize will be taken care of. But multiplying it with a quantity less than one ofcourse takes care of that.
Thanks

Contributor

kartik-nighania commented Aug 18, 2017

@zoq
Ah.. i see. I was thinking of how to make sure less than mutationSize will be taken care of. But multiplying it with a quantity less than one ofcourse takes care of that.
Thanks

@kartik-nighania

This comment has been minimized.

Show comment
Hide comment
@kartik-nighania

kartik-nighania Aug 18, 2017

Contributor

@zoq
I think the PR is complete now. ✌️
Thanks

Contributor

kartik-nighania commented Aug 18, 2017

@zoq
I think the PR is complete now. ✌️
Thanks

* @author Marcus Edel
* @author Kartik Nighania
*
* Conventional Neural Evolution

This comment has been minimized.

@zoq

zoq Aug 20, 2017

Member

Can you elaborate on this, a simple sentence what this method does is just fine.

@zoq

zoq Aug 20, 2017

Member

Can you elaborate on this, a simple sentence what this method does is just fine.

This comment has been minimized.

@kartik-nighania

kartik-nighania Aug 20, 2017

Contributor

done

@kartik-nighania
Show outdated Hide outdated src/mlpack/core/optimizers/cne/cne.hpp
* @author Marcus Edel
* @author Kartik Nighania
*
* Conventional Neural Evolution

This comment has been minimized.

@zoq

zoq Aug 20, 2017

Member

Can you elaborate on this, a simple sentence what this method does is just fine.

@zoq

zoq Aug 20, 2017

Member

Can you elaborate on this, a simple sentence what this method does is just fine.

This comment has been minimized.

@kartik-nighania

kartik-nighania Aug 20, 2017

Contributor

done

@kartik-nighania
Show outdated Hide outdated src/mlpack/core/optimizers/cne/cne_impl.hpp
Show outdated Hide outdated src/mlpack/tests/cne_test.cpp
@zoq

zoq approved these changes Aug 22, 2017

I think this is ready to go, if you can fix the issues I pointed out; let's wait 3 more days before merging it in, in case anyone else has comments.

@kartik-nighania

This comment has been minimized.

Show comment
Hide comment
@kartik-nighania

kartik-nighania Aug 23, 2017

Contributor

thanks Marcus :)

Contributor

kartik-nighania commented Aug 23, 2017

thanks Marcus :)

@rcurtin

Hi Kartik,

I have seen a lot of emails as you worked on this PR, I'm really glad to see it finished, it is clear it is the result of a lot of hard work. :) It all looks great to me, and I really like the detailed documentation. I have some really minor comments, nothing big; if you can handle those it would be great.

Show outdated Hide outdated doc/tutorials/cne/cne.txt
Show outdated Hide outdated doc/tutorials/cne/cne.txt
Show outdated Hide outdated doc/tutorials/cne/cne.txt
Show outdated Hide outdated doc/tutorials/cne/cne.txt
Show outdated Hide outdated doc/tutorials/cne/cne.txt
Show outdated Hide outdated src/mlpack/core/optimizers/cne/cne.hpp
*
* The parameter i refers to which individual function is being evaluated.
* The function will evaluate the objective function on the data point i in
* the dataset.

This comment has been minimized.

@rcurtin

rcurtin Aug 23, 2017

Member

This documentation is great. Thanks so much for the clear details, I feel like I understand the class fully before I have even read the code. :)

@rcurtin

rcurtin Aug 23, 2017

Member

This documentation is great. Thanks so much for the clear details, I feel like I understand the class fully before I have even read the code. :)

This comment has been minimized.

@kartik-nighania

kartik-nighania Aug 24, 2017

Contributor

Cant be anymore happy to hear that. Thanks a lot.
Also the reason I exchanged so many messages is that I am a bad coder 😆 . Constantly learning something from my mentor.
Thanks again 😄

@kartik-nighania

kartik-nighania Aug 24, 2017

Contributor

Cant be anymore happy to hear that. Thanks a lot.
Also the reason I exchanged so many messages is that I am a bad coder 😆 . Constantly learning something from my mentor.
Thanks again 😄

Show outdated Hide outdated src/mlpack/core/optimizers/cne/cne_impl.hpp
Show outdated Hide outdated src/mlpack/core/optimizers/cne/cne_impl.hpp
@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Aug 25, 2017

Member

This looks ready for me, if you don't mind I'd like to squash this into three commits. This would go as follows:

# Backup the code just in case.

git remote add upstream https://github.com/mlpack/mlpack.git
git checkout cne
git fetch upstream master
git reset --soft upstream/master

# Unstage test changes.
git reset HEAD src/mlpack/tests/cne_test.cpp
git reset HEAD src/mlpack/tests/CMakeLists.txt

# Unstage tutorial changes.
git reset HEAD doc/tutorials/README.md
git reset HEAD doc/tutorials/cne/cne.txt
git reset HEAD doc/tutorials/tutorials.txt

# Commit CNE optimizer code.
git commit -m "Add Conventional Neural Evolution (CNE) optimizer."

# Stage test changes.
git add src/mlpack/tests/cne_test.cpp
git add src/mlpack/tests/CMakeLists.txt

# Commit CNE tests.
git commit -m "Add Conventional Neural Evolution (CNE) optimizer test cases."

# Stage tutorial changes.
git add doc/tutorials/README.md
git add doc/tutorials/cne/cne.txt
git add doc/tutorials/tutorials.txt

# Commit CNE tutorials.
git commit -m "Add Conventional Neural Evolution (CNE) optimizer tutorial."

git push --force
Member

zoq commented Aug 25, 2017

This looks ready for me, if you don't mind I'd like to squash this into three commits. This would go as follows:

# Backup the code just in case.

git remote add upstream https://github.com/mlpack/mlpack.git
git checkout cne
git fetch upstream master
git reset --soft upstream/master

# Unstage test changes.
git reset HEAD src/mlpack/tests/cne_test.cpp
git reset HEAD src/mlpack/tests/CMakeLists.txt

# Unstage tutorial changes.
git reset HEAD doc/tutorials/README.md
git reset HEAD doc/tutorials/cne/cne.txt
git reset HEAD doc/tutorials/tutorials.txt

# Commit CNE optimizer code.
git commit -m "Add Conventional Neural Evolution (CNE) optimizer."

# Stage test changes.
git add src/mlpack/tests/cne_test.cpp
git add src/mlpack/tests/CMakeLists.txt

# Commit CNE tests.
git commit -m "Add Conventional Neural Evolution (CNE) optimizer test cases."

# Stage tutorial changes.
git add doc/tutorials/README.md
git add doc/tutorials/cne/cne.txt
git add doc/tutorials/tutorials.txt

# Commit CNE tutorials.
git commit -m "Add Conventional Neural Evolution (CNE) optimizer tutorial."

git push --force
@kartik-nighania

This comment has been minimized.

Show comment
Hide comment
@kartik-nighania

kartik-nighania Aug 26, 2017

Contributor

Done
Thanks @zoq for this guide.
I searched for this on the internet but its a bit messy over there.
Can u tell me a good place to learn about stashing commits like u did?

Contributor

kartik-nighania commented Aug 26, 2017

Done
Thanks @zoq for this guide.
I searched for this on the internet but its a bit messy over there.
Can u tell me a good place to learn about stashing commits like u did?

@kartik-nighania

This comment has been minimized.

Show comment
Hide comment
@kartik-nighania

kartik-nighania Aug 26, 2017

Contributor

@zoq
i changed history.md. dont know if did wrong.
also the following test failed -
nmf_test.cpp

Contributor

kartik-nighania commented Aug 26, 2017

@zoq
i changed history.md. dont know if did wrong.
also the following test failed -
nmf_test.cpp

kartik-nighania added some commits Aug 26, 2017

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Aug 27, 2017

Member

Looks good, thanks for taking care of the issues; I'll merge the code later today. About git: I really like https://try.github.io/ it's not going into all the details, I guess if you run into some problem you still have to search the internet for solutions.

Member

zoq commented Aug 27, 2017

Looks good, thanks for taking care of the issues; I'll merge the code later today. About git: I really like https://try.github.io/ it's not going into all the details, I guess if you run into some problem you still have to search the internet for solutions.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Aug 27, 2017

Member

Thanks for the great contribution!

Merged: 7af5fd1 99ce3b9 49ff33b

Member

zoq commented Aug 27, 2017

Thanks for the great contribution!

Merged: 7af5fd1 99ce3b9 49ff33b

@zoq zoq closed this Aug 27, 2017

@kartik-nighania

This comment has been minimized.

Show comment
Hide comment
@kartik-nighania

kartik-nighania Aug 27, 2017

Contributor

wow!
my first open source merge..

thanks a lot for your help.
@zoq

Contributor

kartik-nighania commented Aug 27, 2017

wow!
my first open source merge..

thanks a lot for your help.
@zoq

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Aug 27, 2017

Member

That reminds me of something, feel free to open a new PR to add yourself to the contributors list: COPYRIGHT.txt and src/mlpack/core.hpp

Member

zoq commented Aug 27, 2017

That reminds me of something, feel free to open a new PR to add yourself to the contributors list: COPYRIGHT.txt and src/mlpack/core.hpp

@kartik-nighania

This comment has been minimized.

Show comment
Hide comment
@kartik-nighania

kartik-nighania Aug 27, 2017

Contributor

@zoq
I think Marcus its too early that i deserve to be added in contributors now..
Ill keep working for now and would add some time later..
I hope thats okay.
:)

Contributor

kartik-nighania commented Aug 27, 2017

@zoq
I think Marcus its too early that i deserve to be added in contributors now..
Ill keep working for now and would add some time later..
I hope thats okay.
:)

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Aug 27, 2017

Member

I think Marcus its too early that i deserve to be added in contributors now..

I don't think that's true, your contributions are much appreciated.

Member

zoq commented Aug 27, 2017

I think Marcus its too early that i deserve to be added in contributors now..

I don't think that's true, your contributions are much appreciated.

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Aug 28, 2017

Member

Typically we have added people after they have contributed a single line of code to mlpack, so please feel free to add yourself. :)

Member

rcurtin commented Aug 28, 2017

Typically we have added people after they have contributed a single line of code to mlpack, so please feel free to add yourself. :)

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