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

Differential Evolution #77

Merged
merged 14 commits into from Feb 20, 2019

Conversation

@favre49
Copy link
Member

commented Feb 2, 2019

This is a best/1/bin implementation of Differential Evolution

DE
* @param differentialWeight A parameter used in the mutation of candidate solutions,
* controls amplification factor of the differentiation.
*/
DE(const size_t populationSize = 100,

This comment has been minimized.

Copy link
@zoq

zoq Feb 5, 2019

Member

This looks off, can you take a look at the style format?

* @param iterate Starting point (will be modified).
* @return Objective value of the final point.
*/
template<typename DecomposableFunctionType>

This comment has been minimized.

Copy link
@zoq

zoq Feb 5, 2019

Member

See comment above.

double differentialWeight;

};

This comment has been minimized.

Copy link
@zoq

zoq Feb 5, 2019

Member

Looks like we can remove the empty line here.


namespace ens {

inline DE::DE( const size_t populationSize ,

This comment has been minimized.

Copy link
@zoq

zoq Feb 5, 2019

Member

Can you adapt the style from the existing codebase? For more information see https://github.com/mlpack/mlpack/wiki/DesignGuidelines.

This comment has been minimized.

Copy link
@favre49

favre49 Feb 6, 2019

Author Member

I had not seen this, i will make the necessary fixes

for(int i = 0; i < populationSize; i++)
{
arma::mat slicer = population.slice(i);
slicer = iterate+slicer;

This comment has been minimized.

Copy link
@zoq

zoq Feb 5, 2019

Member

Hm, looks like we can avoid the copy here and directly use population.slice(i) = population.slice(i) + iterate.

fitnessValues[it] = function.Evaluate(population.slice(it));
double lastBestFitness = fitnessValues.min();
arma::mat bestElement;
for(int it = 0; it < populationSize; it++)

This comment has been minimized.

Copy link
@zoq

zoq Feb 5, 2019

Member

Do you think we could move this into the for loop above, so right after we do the Evaluate we can check if the fitness improved.

@favre49

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

I have made the changes to the code style and have added your suggestions

#ifndef ENSMALLEN_DE_DE_HPP
#define ENSMALLEN_DE_DE_HPP

namespace ens{

This comment has been minimized.

Copy link
@zoq

zoq Feb 11, 2019

Member

Pedantic style issue; missing space after ens.

* documentation on function types included with this distribution or on the
* ensmallen website.
*/

This comment has been minimized.

Copy link
@zoq

zoq Feb 11, 2019

Member

Let's remove the empty line here.

* solutions controls amplification factor of the differentiation.
*/
DE(const size_t populationSize = 100,
const size_t maxGenerations = 2000,

This comment has been minimized.

Copy link
@zoq

zoq Feb 11, 2019

Member

Another picky style issue; the parameter should align with the first one.

#include "de.hpp"
#include <random>
#include <tuple>
#include <iostream>

This comment has been minimized.

Copy link
@zoq

zoq Feb 11, 2019

Member

I think we can remove each header besides de.hpp.

namespace ens {

inline DE::DE(const size_t populationSize ,
const size_t maxGenerations,

This comment has been minimized.

Copy link
@zoq

zoq Feb 11, 2019

Member

The parameter should align with the first one.

{
// Population Size must be atleast 3 for DE to work
if (populationSize < 3)
{

This comment has been minimized.

Copy link
@zoq

zoq Feb 11, 2019

Member

The brace placement is off; see https://github.com/mlpack/mlpack/wiki/DesignGuidelines#brace-placement for more information.

// Generate a population based on a Gaussian distribution around the given starting point.
// Also finds the best element of the population
population = arma::randn(iterate.n_rows, iterate.n_cols, populationSize);
for(int i = 0; i < populationSize; i++)

This comment has been minimized.

Copy link
@zoq

zoq Feb 11, 2019

Member

To be consistent with the rest of the codebase, let's use size_t instead of int to loop over the matrix.

int l=0, m=0;
do
{
l = dist(rng);

This comment has been minimized.

Copy link
@zoq

zoq Feb 11, 2019

Member

Do you mind to use arma::as_scalar(arma::randi<arma::uvec>(1, arma::distr_param(0, populationSize - 1))); here, same for the selction of m?

// Generate new "mutant" from two randomly chosen members
arma::mat a = population.slice(l);
arma::mat b = population.slice(m);
arma::mat mutant = bestElement + differentialWeight*(a - b);

This comment has been minimized.

Copy link
@zoq

zoq Feb 11, 2019

Member

Missing space between * operation.

@favre49

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

I have made the changes you suggested

namespace ens {

/**
* Differential evolution is a stochastic evolutionary algorithm used for global optimization.

This comment has been minimized.

Copy link
@zoq

zoq Feb 14, 2019

Member

Do you think you could cite the "Differential Evolution - a Simple and Efficient Adaptive Scheme for Global Optimization over Continuous Spaces" paper; example:

* @code
* @article{
* title = {Closing the Generalization Gap of Adaptive Gradient Methods in
* Training Deep Neural Networks},
* author = {{Chen}, J. and {Gu}, Q.},
* journal = {ArXiv e-prints},
* url = {https://arxiv.org/abs/1806.06763}
* year = {2018}
* }
* @endcode

@favre49

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

i have added the citation

@zoq

zoq approved these changes Feb 15, 2019

Copy link
Member

left a comment

Looks good to me, there are some style issues left, I can handle that during the merge process. Another thing we should do (or that I can do during the merge process) is to add a line to HISTORY.md file.

favre49 added some commits Feb 16, 2019

@zoq

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

Can you revert the last two commits?

favre49 added some commits Feb 18, 2019

Revert "fix to issue #80"
This reverts commit 0c8ae9d.
@favre49

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2019

Yup, sorry about that, i had committed it by mistake. Still new to git

@zoq

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

No worries, once the build comes back green, I'll hit the merge button.

@rcurtin
Copy link
Member

left a comment

A couple of little comments, none major. I think these could be handled during merge if @zoq wants to do it there. 👍

Also feel free to add something to HISTORY.md.

Thanks for the contribution!

| **type** | **name** | **description** | **default** |
|----------|----------|-----------------|-------------|
| `size_t` | **`populationSize`** | The number of candidates in the population. This should be at least 3 in size. | `100` |
| `size_t` | **`maxGenerations`** | The maximum number of generations allowed for CNE. | `2000` |

This comment has been minimized.

Copy link
@rcurtin

rcurtin Feb 18, 2019

Member

Should this be DE not CNE?

lastBestFitness = fitnessValues.min();
for (size_t it = 0; it < populationSize; it++)
if (fitnessValues[it] == lastBestFitness)
bestElement = population.slice(it);

This comment has been minimized.

Copy link
@rcurtin

rcurtin Feb 18, 2019

Member

Could we use index_min() here?

This comment has been minimized.

Copy link
@favre49

favre49 Feb 18, 2019

Author Member

arma::vec doesn't have a index_min() member function, and if i try to use the standalone function it gives me the error "index_min() is not a member of arma". That's why i did it this way.

This comment has been minimized.

Copy link
@rcurtin

rcurtin Feb 18, 2019

Member

You should be able to fully specify the name with arma::index_min(). :)

This comment has been minimized.

Copy link
@favre49

favre49 Feb 18, 2019

Author Member

Yes, doing that is giving me that error.
The offending line of code:
arma::uword index = arma::index_min(fitnessValues);
The error message:
error: ‘index_min’ is not a member of ‘arma’

This comment has been minimized.

Copy link
@rcurtin

rcurtin Feb 18, 2019

Member

Oh, sorry, this is because the minimum supported version of Armadillo doesn't have that. Sorry about that. In this case the loop you have is fine but you might consider adding a break inside the if so it doesn't perform unnecessary iterations. 👍

using namespace ens::test;

/**
* Train and test a logistic regression function using CNE optimizer

This comment has been minimized.

Copy link
@rcurtin

rcurtin Feb 18, 2019

Member

I think this should read DE optimizer. :)

@@ -0,0 +1,41 @@
/**
* @file cne_test.cpp

This comment has been minimized.

Copy link
@rcurtin

rcurtin Feb 18, 2019

Member

I think this should be updated also. 👍

@@ -0,0 +1,129 @@
/**
* @file de.hpp

This comment has been minimized.

Copy link
@rcurtin

rcurtin Feb 18, 2019

Member

de_impl.hpp :)

favre49 added some commits Feb 18, 2019

@zoq zoq merged commit 555a8b7 into mlpack:master Feb 20, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zoq

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

Thanks again for the contribution; If you'd like, we can mail you some mlpack stickers that you can put on your laptop. If you want some, just send an email with your mailing address to stickers@mlpack.org and I will put them in an envelope and get them sent. We can also add your name to the list of contributors if you like---just let me know.

@favre49

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2019

Thanks for your help :) Please add me to the list of contributors

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.