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

CNE algorithm #753

Closed
wants to merge 74 commits into
from

Conversation

Projects
None yet
5 participants
@BangLiu

BangLiu commented Aug 5, 2016

Implemented CNE algorithm, and XOR test case.

BangLiu added some commits May 17, 2016

CNE algorithm finished. XOR test passes. Need to revise coding style …
…and some TODOs. Then continue other algorithms.
seems bugs solved. keep printf. Seems information such as species siz…
…e, numSpecies are not updated when things changed, that is why so many bugs.
currently seems no bug. Previously we removed stale species even when…
… species number is small, leads to species disappear.
Removed redundant members in species, population. haven't remove in g…
…enome, as it caused some bug. Still need checking.
changed the place where we should set the childGenome's NumInput, Num…
…Output, GenomeDepth. However, seems calculate genome depth is quite slow.
Show outdated Hide outdated src/mlpack/methods/ne/link_gene.hpp
class LinkGene {
public:
// Default constructor.
LinkGene() {}

This comment has been minimized.

@zoq

zoq Aug 7, 2016

Member

Do we have to specify the default constructor?

@zoq

zoq Aug 7, 2016

Member

Do we have to specify the default constructor?

This comment has been minimized.

@BangLiu

BangLiu Aug 7, 2016

Yeah, maybe useless. I can remove it.

@BangLiu

BangLiu Aug 7, 2016

Yeah, maybe useless. I can remove it.

Show outdated Hide outdated src/mlpack/methods/ne/genome.hpp
#ifndef MLPACK_METHODS_NE_GENOME_HPP
#define MLPACK_METHODS_NE_GENOME_HPP
#include <cstddef>

This comment has been minimized.

@zoq

zoq Aug 7, 2016

Member

I think we don't need this header file anymore.

@zoq

zoq Aug 7, 2016

Member

I think we don't need this header file anymore.

This comment has been minimized.

Show outdated Hide outdated src/mlpack/methods/ne/cne.hpp
}
// Destructor.
~CNE() {}

This comment has been minimized.

@zoq

zoq Aug 7, 2016

Member

I think there is no need to implement a default destructor.

@zoq

zoq Aug 7, 2016

Member

I think there is no need to implement a default destructor.

Show outdated Hide outdated src/mlpack/methods/ne/cne.hpp
class CNE {
public:
// Parametric constructor.
CNE(TaskType task, Genome& seedGenome, Parameters& params) {

This comment has been minimized.

@zoq

zoq Aug 7, 2016

Member

It would be great if you could provide a Constructor that allows a user to directly set the parameters, instead of passing a parameter object.

@zoq

zoq Aug 7, 2016

Member

It would be great if you could provide a Constructor that allows a user to directly set the parameters, instead of passing a parameter object.

This comment has been minimized.

@zoq

zoq Aug 7, 2016

Member

It might be useful to use const for all parameters.

@zoq

zoq Aug 7, 2016

Member

It might be useful to use const for all parameters.

This comment has been minimized.

@BangLiu

BangLiu Aug 7, 2016

I think for algorithms the parameters are a lot. Users can set parameter by construct an Parameters object without put them into a constructor function in a specific sequence. And this also makes the algorithm constructor be concise. If we have more parameters in the future, we just add new member for Parameters class, no need to change the function headers of constructors. Besides, different NE algorithm will have similar functions for constructor.

@BangLiu

BangLiu Aug 7, 2016

I think for algorithms the parameters are a lot. Users can set parameter by construct an Parameters object without put them into a constructor function in a specific sequence. And this also makes the algorithm constructor be concise. If we have more parameters in the future, we just add new member for Parameters class, no need to change the function headers of constructors. Besides, different NE algorithm will have similar functions for constructor.

This comment has been minimized.

@zoq

zoq Aug 8, 2016

Member

I agree, that for some methods you have to specify a bunch of parameters, so I think the parameter class comes in handy. On the other side, I like to provide a constructor that works right out of the box and starts with some reasonable parameters. I think in the case of the CNE method, we can specify such parameters. Maybe we can provide an constructor for such a situation, what do you think?

@zoq

zoq Aug 8, 2016

Member

I agree, that for some methods you have to specify a bunch of parameters, so I think the parameter class comes in handy. On the other side, I like to provide a constructor that works right out of the box and starts with some reasonable parameters. I think in the case of the CNE method, we can specify such parameters. Maybe we can provide an constructor for such a situation, what do you think?

Show outdated Hide outdated src/mlpack/methods/ne/cne.hpp
// Soft mutation: add a random value chosen from
// initialization prob distribution with probability p.
// TODO: here we use uniform distribution. Can we use exponential distribution?

This comment has been minimized.

@zoq

zoq Aug 7, 2016

Member

I guess for now it's reasonable to use a uniform distribution only.

@zoq

zoq Aug 7, 2016

Member

I guess for now it's reasonable to use a uniform distribution only.

This comment has been minimized.

@BangLiu

BangLiu Aug 7, 2016

Yeah, I agree.

@BangLiu

BangLiu Aug 7, 2016

Yeah, I agree.

Show outdated Hide outdated src/mlpack/methods/ne/cne.hpp
// Hard mutation: replace with a random value chosen from
// initialization prob distribution with probability p.
// TODO: here we use uniform distribution. Can we use exponential distribution?
static void MutateWeightsUnbiased(Genome& genome, double mutateProb, double mutateSize) {

This comment has been minimized.

@zoq

zoq Aug 7, 2016

Member

Do you think, we could use a boolean to distinguish between soft and hard mutation. In this case we could reuse the basic function.

@zoq

zoq Aug 7, 2016

Member

Do you think, we could use a boolean to distinguish between soft and hard mutation. In this case we could reuse the basic function.

This comment has been minimized.

@BangLiu

BangLiu Aug 7, 2016

Yeah, we can do this.

@BangLiu

BangLiu Aug 7, 2016

Yeah, we can do this.

Show outdated Hide outdated src/mlpack/methods/ne/cne.hpp
aSpecies.SetBestFitnessAndGenome();
// Output some information.
printf("Generation: %zu\tBest fitness: %f\n", generation, aSpecies.BestFitness());

This comment has been minimized.

@zoq

zoq Aug 7, 2016

Member

I think it would be better to use Log::Info here, in that case a user can disable any output e.g:

Log::Info << "Generation: " << generation << " best fitness: " <<  species.BestFitness() << std::endl

or something like that, the important thing is a user can disable the output.

@zoq

zoq Aug 7, 2016

Member

I think it would be better to use Log::Info here, in that case a user can disable any output e.g:

Log::Info << "Generation: " << generation << " best fitness: " <<  species.BestFitness() << std::endl

or something like that, the important thing is a user can disable the output.

This comment has been minimized.

@BangLiu

BangLiu Aug 7, 2016

Good idea, I will revise.

@BangLiu

BangLiu Aug 7, 2016

Good idea, I will revise.

Show outdated Hide outdated src/mlpack/methods/ne/cne.hpp
printf("Generation: %zu\tBest fitness: %f\n", generation, aSpecies.BestFitness());
if (aTask.Success()) {
printf("Task succeed in %zu iterations.\n", generation);
exit(0);

This comment has been minimized.

@zoq

zoq Aug 7, 2016

Member

That would terminate the complete process, I would go with return true.

@zoq

zoq Aug 7, 2016

Member

That would terminate the complete process, I would go with return true.

This comment has been minimized.

@BangLiu

BangLiu Aug 7, 2016

Revised for both algorithm: cne and neat.

@BangLiu

BangLiu Aug 7, 2016

Revised for both algorithm: cne and neat.

Show outdated Hide outdated src/mlpack/methods/ne/species.hpp
return;
aBestFitness = aGenomes[0].Fitness();
for (int i=0; i<aGenomes.size(); ++i) {

This comment has been minimized.

@zoq

zoq Aug 7, 2016

Member

We can start with i = 1, also right now it's possible that aBestGenome is not set if the the first genome is the genome with the best fitness.

@zoq

zoq Aug 7, 2016

Member

We can start with i = 1, also right now it's possible that aBestGenome is not set if the the first genome is the genome with the best fitness.

This comment has been minimized.

@BangLiu

BangLiu Aug 8, 2016

If the species only contains one genome, start from i=1 will cause error. So I think we keep start from 0.

@BangLiu

BangLiu Aug 8, 2016

If the species only contains one genome, start from i=1 will cause error. So I think we keep start from 0.

This comment has been minimized.

@zoq

zoq Aug 8, 2016

Member

In that case aGenomes.size() is 1 and we don't enter the for loop, right? Also, what do you think about the unset aBestGenome problem if genome with index 0 has the best fitness?

@zoq

zoq Aug 8, 2016

Member

In that case aGenomes.size() is 1 and we don't enter the for loop, right? Also, what do you think about the unset aBestGenome problem if genome with index 0 has the best fitness?

Show outdated Hide outdated src/mlpack/methods/ne/species.hpp
}
// Sort genomes by fitness. First is best.
static bool CompareGenome(Genome lg, Genome rg) {

This comment has been minimized.

@zoq

zoq Aug 7, 2016

Member

We should use a reference here, to avoid a copy, also it might be useful to use const here.

@zoq

zoq Aug 7, 2016

Member

We should use a reference here, to avoid a copy, also it might be useful to use const here.

This comment has been minimized.

@BangLiu

BangLiu Aug 8, 2016

Revised.

Show outdated Hide outdated src/mlpack/methods/ne/utils.hpp
*/
// Return the sign of a number.
template <typename T> int sgn(T val) {

This comment has been minimized.

@zoq

zoq Aug 7, 2016

Member

What about using arma::sign(val) take a look at: http://arma.sourceforge.net/docs.html#misc_fns for more informations.

@zoq

zoq Aug 7, 2016

Member

What about using arma::sign(val) take a look at: http://arma.sourceforge.net/docs.html#misc_fns for more informations.

Show outdated Hide outdated src/mlpack/methods/ne/utils.hpp
}
// Return a random float between [0, 1]
double RandFloat() {

This comment has been minimized.

@zoq

zoq Aug 7, 2016

Member

It looks like this function is unused, so maybe we can remove it?

@zoq

zoq Aug 7, 2016

Member

It looks like this function is unused, so maybe we can remove it?

This comment has been minimized.

@BangLiu

BangLiu Aug 8, 2016

Yeah, no problem.

@BangLiu

BangLiu Aug 8, 2016

Yeah, no problem.

@@ -0,0 +1,691 @@
/**
* @file ne_test.cpp

This comment has been minimized.

@zoq

zoq Aug 7, 2016

Member

Can you split this test into smaller parts. Like link_gene_test.cpp, neuron_gene_test.cpp, genome_test.cpp, cne_test.cpp, neat_test.cpp.

@zoq

zoq Aug 7, 2016

Member

Can you split this test into smaller parts. Like link_gene_test.cpp, neuron_gene_test.cpp, genome_test.cpp, cne_test.cpp, neat_test.cpp.

This comment has been minimized.

@BangLiu

BangLiu Aug 8, 2016

I think it would be better if we keep the tests of NE module in the same file.

@BangLiu

BangLiu Aug 8, 2016

I think it would be better if we keep the tests of NE module in the same file.

This comment has been minimized.

@zoq

zoq Aug 8, 2016

Member

You mean all NE tests? I'm not sure, if you implement another NE algorithm or like to test an NE methods on another task, the test file gets longer and longer. If you split the test file, and someone is only interested in the NEAT method, he could just take a look at the NEAT test file. let me know what you think. If you like the idea of a single test file, we can go with that solution.

@zoq

zoq Aug 8, 2016

Member

You mean all NE tests? I'm not sure, if you implement another NE algorithm or like to test an NE methods on another task, the test file gets longer and longer. If you split the test file, and someone is only interested in the NEAT method, he could just take a look at the NEAT test file. let me know what you think. If you like the idea of a single test file, we can go with that solution.

species.SetBestFitnessAndGenome();
// Output some information.
printf("Generation: %zu\tBest fitness: %f\n", generation, species.BestFitness());

This comment has been minimized.

@zoq

zoq Aug 29, 2016

Member

Can you use Log::Info << "message" << std::endl; instead of printf? That way a user can disable the output if needed or adjust the output according to his own preference e.g. only debug outputs (Log::Debug).

@zoq

zoq Aug 29, 2016

Member

Can you use Log::Info << "message" << std::endl; instead of printf? That way a user can disable the output if needed or adjust the output according to his own preference e.g. only debug outputs (Log::Debug).

std::vector<NeuronGene> neuronGenes;
std::vector<LinkGene> linkGenes;
NeuronGene inputGene1(0, INPUT, LINEAR, 0, std::vector<double>(), 0, 0);

This comment has been minimized.

@zoq

zoq Aug 29, 2016

Member

Do you think, we could set a default argument for the coordinateparameter. That way we don't have to specify std::vector<double>() every time we create a new NeuronGene.

@zoq

zoq Aug 29, 2016

Member

Do you think, we could set a default argument for the coordinateparameter. That way we don't have to specify std::vector<double>() every time we create a new NeuronGene.

fitness);
// Test seed genome.
std::vector<std::vector<double>> inputs; // TODO: use arma::mat for input.

This comment has been minimized.

@zoq

zoq Aug 29, 2016

Member

It would be great if we could to that change before we merge the code in. If you like I can do that for you.

@zoq

zoq Aug 29, 2016

Member

It would be great if we could to that change before we merge the code in. If you like I can do that for you.

/**
* Return the sign of a number.
*/
template <typename T> int sgn(T val)

This comment has been minimized.

@zoq

zoq Aug 29, 2016

Member

If we use armadillo's sgn function, we could remove the utils file, or do we need another function from the header file?

@zoq

zoq Aug 29, 2016

Member

If we use armadillo's sgn function, we could remove the utils file, or do we need another function from the header file?

@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?

Can one of the admins verify this patch?

@sourabhvarshney111

This comment has been minimized.

Show comment
Hide comment
@sourabhvarshney111

sourabhvarshney111 Mar 17, 2018

Contributor

Can @zoq @rcurtin verify what is the issue with this?

Contributor

sourabhvarshney111 commented Mar 17, 2018

Can @zoq @rcurtin verify what is the issue with this?

@zoq

This comment has been minimized.

Show comment
Hide comment
Member

zoq commented Mar 17, 2018

@zoq zoq closed this Mar 17, 2018

@sourabhvarshney111

This comment has been minimized.

Show comment
Hide comment
@sourabhvarshney111

sourabhvarshney111 Mar 17, 2018

Contributor

Thanks @zoq :)

Contributor

sourabhvarshney111 commented Mar 17, 2018

Thanks @zoq :)

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