Genome mutate - implements new logic for mutations #180

HenrikMettler · 2020-07-10T14:23:56Z

Implements the new logic for mutation discussed in #172. Closes #172 , #173 and makes #124 obsolete. Does not pass test_parallel_population in test/test_hl_api.py, since fitnes is not the with 1 or 2 parallel processes. I created the PR anyway since I couldn't figure out why and whether this is a problem with the new logic in mutate

mschmidt87

Can you rebase your branch on current master? I think you branched off a while ago and now there are some changes in the PR which don't belong here.

cgp/genome.py

jakobj

awesome, great work @HenrikMettler! 🚀

this is quite a substantial change, so accordingly I had a few remarks. ;) they should be addressed before merging. please let me know if anything is unclear.

cgp/genome.py

test/test_genome.py

jakobj · 2020-07-14T20:26:27Z

[snip] Does not pass test_parallel_population in test/test_hl_api.py, since fitnes is not the with 1 or 2 parallel processes. I created the PR anyway since I couldn't figure out why and whether this is a problem with the new logic in mutate

the fitness should be identical, independent of the number of processes as far as I remember. this is obviously desirable from a reproducibility viewpoint. is it possible that any of the stochastic operations are not using the internal rng of the population? that would be my first guess.

HenrikMettler · 2020-07-16T09:50:26Z

Just noticed that test_create_new_offspring_and_parent_generation in test_ea_mu_plus_lambda.py occasionally fails. I assume this is since non of the genes that have more than one permissible value is selected for mutation. Should I adapt the test logic or adapt mutate such that always at least one gene in mutated?

HenrikMettler · 2020-07-16T10:42:32Z

the fitness should be identical, independent of the number of processes as far as I remember. this is obviously desirable from a reproducibility viewpoint. is it possible that any of the stochastic operations are not using the internal rng of the population? that would be my first guess.

As far as I can see the problem just arises when setting n_process = 1 and not for any other (valid) number of processes. Could the error have something to do with https://github.com/Happy-Algorithms-League/hal-cgp/blob/master/cgp/ea/mu_plus_lambda.py#L157 ? I don't know how my changes influence this though

jakobj · 2020-07-16T10:44:40Z

Just noticed that test_create_new_offspring_and_parent_generation in test_ea_mu_plus_lambda.py occasionally fails. I assume this is since non of the genes that have more than one permissible value is selected for mutation. Should I adapt the test logic or adapt mutate such that always at least one gene in mutated?

i would adapt the test logic as we are happy with the logic in mutate. maybe you can increase the mutation rate to 1.0?

jakobj · 2020-07-16T10:45:26Z

the fitness should be identical, independent of the number of processes as far as I remember. this is obviously desirable from a reproducibility viewpoint. is it possible that any of the stochastic operations are not using the internal rng of the population? that would be my first guess.

As far as I can see the problem just arises when setting n_process = 1 and not for any other (valid) number of processes. Could the error have something to do with https://github.com/Happy-Algorithms-League/hal-cgp/blob/master/cgp/ea/mu_plus_lambda.py#L157 ? I don't know how my changes influence this though

what do you mean by "the problem"? is the fitness always the same except for running a single process?

jakobj

this is improving nicely! just added two more comments

cgp/genome.py

test/test_genome.py

HenrikMettler · 2020-07-16T12:14:55Z

Just noticed that test_create_new_offspring_and_parent_generation in test_ea_mu_plus_lambda.py occasionally fails. I assume this is since non of the genes that have more than one permissible value is selected for mutation. Should I adapt the test logic or adapt mutate such that always at least one gene in mutated?

i would adapt the test logic as we are happy with the logic in mutate. maybe you can increase the mutation rate to 1.0?

I can't set it to '1.0' because of https://github.com/Happy-Algorithms-League/hal-cgp/blob/master/cgp/population.py#L37 (if not (0.0 < mutation_rate and mutation_rate < 1.0): raise ValueError("mutation rate needs to be in (0, 1)") But 0.99 should be fine, I guess? (The chance of not having 0 mutations with that mutation_rate can be neglectable)

HenrikMettler · 2020-07-16T12:18:34Z

the fitness should be identical, independent of the number of processes as far as I remember. this is obviously desirable from a reproducibility viewpoint. is it possible that any of the stochastic operations are not using the internal rng of the population? that would be my first guess.

As far as I can see the problem just arises when setting n_process = 1 and not for any other (valid) number of processes. Could the error have something to do with https://github.com/Happy-Algorithms-League/hal-cgp/blob/master/cgp/ea/mu_plus_lambda.py#L157 ? I don't know how my changes influence this though

what do you mean by "the problem"? is the fitness always the same except for running a single process?

Yes. (It also doesn't matter at which position in the vector n_process = 1 is, eg if I set for n_process in [2, 1, 4] still the one where n_process = 1 is having a different fitness)

jakobj · 2020-07-17T09:07:11Z

i would adapt the test logic as we are happy with the logic in mutate. maybe you can increase the mutation rate to 1.0?

I can't set it to '1.0' because of https://github.com/Happy-Algorithms-League/hal-cgp/blob/master/cgp/population.py#L37 (if not (0.0 < mutation_rate and mutation_rate < 1.0): raise ValueError("mutation rate needs to be in (0, 1)") But 0.99 should be fine, I guess? (The chance of not having 0 mutations with that mutation_rate can be neglectable)

ah, interesting. well, why don't we allow a mutation rate of 1.0? excluding the lower bound makes sense since no evolution takes place if no mutation can happen, but the upper bound could be included, right?

jakobj · 2020-07-17T09:12:02Z

the fitness should be identical, independent of the number of processes as far as I remember. this is obviously desirable from a reproducibility viewpoint. is it possible that any of the stochastic operations are not using the internal rng of the population? that would be my first guess.

As far as I can see the problem just arises when setting n_process = 1 and not for any other (valid) number of processes. Could the error have something to do with https://github.com/Happy-Algorithms-League/hal-cgp/blob/master/cgp/ea/mu_plus_lambda.py#L157 ? I don't know how my changes influence this though

what do you mean by "the problem"? is the fitness always the same except for running a single process?

Yes. (It also doesn't matter at which position in the vector n_process = 1 is, eg if I set for n_process in [2, 1, 4] still the one where n_process = 1 is having a different fitness)

this is not good and we should fix it before merging. i'll see whether i find time today to look into this.

jakobj

this looks great, nice solution to the only_silent_mutations testing 🚀

added a few more comments

cgp/genome.py

test/test_genome.py

cgp/genome.py

jakobj

added a few inline comments regarding the generation of random numbers

cgp/genome.py

jakobj

looking good, just some more nitpicking :)

cgp/genome.py

test/test_genome.py

test/test_hl_api.py

test/test_genome.py

jakobj

alright, four more small comments to fix and this is good to go! :)

you also need to rebase on the current master to fix the conflicts before we can merge

test/test_genome.py

cgp/genome.py

HenrikMettler · 2020-07-20T14:17:45Z

I am not sure how to fix to travis CI build issue, when I introduce a line break, black "refixes" it

cgp/genome.py

test/test_genome.py

jakobj · 2020-07-20T15:45:10Z

I am not sure how to fix to travis CI build issue, when I introduce a line break, black "refixes" it

hmm, yeah, black and flake seem to bite each other here. i suggested a shorter variable name, that should do the trick :)

… random prob is below the mutation rate, evaluate permissible values for every gene, make a mutation always change the gene value, if more than one value is possible for this gene. Add test for number of mutations in a large population (must be close to expected value), test only silent mutation by monkey patching the indices selection function, testing permissible values for hidden and output region. Allow mutation_rates = 1.0.

jakobj

great work, this look good 🎉 ✨

approving 👍 conditioned on travis passing

mschmidt87 reviewed Jul 12, 2020

View reviewed changes

cgp/genome.py Outdated Show resolved Hide resolved

cgp/genome.py Outdated Show resolved Hide resolved

cgp/genome.py Outdated Show resolved Hide resolved

HenrikMettler force-pushed the issue/assert_real_mutations branch 3 times, most recently from 47f99b8 to a8034c1 Compare July 14, 2020 11:58

jakobj added this to the 0.2.0 milestone Jul 14, 2020

jakobj requested changes Jul 14, 2020

View reviewed changes

jakobj reviewed Jul 16, 2020

View reviewed changes

cgp/genome.py Outdated Show resolved Hide resolved

test/test_genome.py Outdated Show resolved Hide resolved

jakobj requested changes Jul 17, 2020

View reviewed changes

mschmidt87 reviewed Jul 18, 2020

View reviewed changes

cgp/genome.py Show resolved Hide resolved

jakobj reviewed Jul 19, 2020

View reviewed changes

cgp/genome.py Outdated Show resolved Hide resolved

cgp/genome.py Outdated Show resolved Hide resolved

cgp/genome.py Outdated Show resolved Hide resolved

mschmidt87 mentioned this pull request Jul 20, 2020

Investigate moving determine_alternative_permissible_values function for genes to __init__ of Genome. #196

Closed

HenrikMettler force-pushed the issue/assert_real_mutations branch from 7efe834 to 742cc9f Compare July 20, 2020 08:09

This was linked to issues Jul 20, 2020

only_silent_mutations has false negatives #173

Closed

Number of mutations should not be fixed to average #124

Closed

jakobj requested changes Jul 20, 2020

View reviewed changes

jakobj reviewed Jul 20, 2020

View reviewed changes

test/test_genome.py Outdated Show resolved Hide resolved

jakobj requested changes Jul 20, 2020

View reviewed changes

test/test_genome.py Outdated Show resolved Hide resolved

test/test_genome.py Outdated Show resolved Hide resolved

test/test_genome.py Show resolved Hide resolved

cgp/genome.py Outdated Show resolved Hide resolved

jakobj reviewed Jul 20, 2020

View reviewed changes

cgp/genome.py Outdated Show resolved Hide resolved

jakobj reviewed Jul 20, 2020

View reviewed changes

test/test_genome.py Outdated Show resolved Hide resolved

jakobj reviewed Jul 20, 2020

View reviewed changes

test/test_genome.py Outdated Show resolved Hide resolved

HenrikMettler force-pushed the issue/assert_real_mutations branch from 863ed1e to 11343f1 Compare July 21, 2020 10:37

HenrikMettler force-pushed the issue/assert_real_mutations branch from 11343f1 to 66393bc Compare July 21, 2020 10:46

jakobj approved these changes Jul 21, 2020

View reviewed changes

jakobj merged commit 607d028 into Happy-Algorithms-League:master Jul 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Genome mutate - implements new logic for mutations #180

Genome mutate - implements new logic for mutations #180

HenrikMettler commented Jul 10, 2020

mschmidt87 left a comment

jakobj left a comment

jakobj commented Jul 14, 2020

HenrikMettler commented Jul 16, 2020

HenrikMettler commented Jul 16, 2020

jakobj commented Jul 16, 2020

jakobj commented Jul 16, 2020

jakobj left a comment

HenrikMettler commented Jul 16, 2020

HenrikMettler commented Jul 16, 2020

jakobj commented Jul 17, 2020

jakobj commented Jul 17, 2020

jakobj left a comment

jakobj left a comment

jakobj left a comment

jakobj left a comment

HenrikMettler commented Jul 20, 2020

jakobj commented Jul 20, 2020

jakobj left a comment

Genome mutate - implements new logic for mutations #180

Genome mutate - implements new logic for mutations #180

Conversation

HenrikMettler commented Jul 10, 2020

mschmidt87 left a comment

Choose a reason for hiding this comment

jakobj left a comment

Choose a reason for hiding this comment

jakobj commented Jul 14, 2020

HenrikMettler commented Jul 16, 2020

HenrikMettler commented Jul 16, 2020

jakobj commented Jul 16, 2020

jakobj commented Jul 16, 2020

jakobj left a comment

Choose a reason for hiding this comment

HenrikMettler commented Jul 16, 2020

HenrikMettler commented Jul 16, 2020

jakobj commented Jul 17, 2020

jakobj commented Jul 17, 2020

jakobj left a comment

Choose a reason for hiding this comment

jakobj left a comment

Choose a reason for hiding this comment

jakobj left a comment

Choose a reason for hiding this comment

jakobj left a comment

Choose a reason for hiding this comment

HenrikMettler commented Jul 20, 2020

jakobj commented Jul 20, 2020

jakobj left a comment

Choose a reason for hiding this comment