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

Fix counting of mutations in genome.mutate #157

Conversation

HenrikMettler
Copy link
Contributor

This PR fixes the mutation counter by explicitly comparing the number of differences in dna before and after mutations

fixes #156

@jakobj jakobj requested a review from mschmidt87 June 24, 2020 10:34
@jakobj jakobj added this to the 0.1.1 milestone Jun 24, 2020
test/test_genome.py Outdated Show resolved Hide resolved
test/test_genome.py Outdated Show resolved Hide resolved
test/test_genome.py Outdated Show resolved Hide resolved
@jakobj
Copy link
Member

jakobj commented Jun 26, 2020

thanks for the update ✨ unfortunately travis is unhappy with the new changes, could you please have a look and fix the issues?

Copy link
Member

@mschmidt87 mschmidt87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for fixing this, I just have some small suggestions.

Actually, don't we need to adapt the parameters set in the examples with this change, @jakobj ?

cgp/genome.py Outdated Show resolved Hide resolved
test/test_genome.py Show resolved Hide resolved
test/test_genome.py Show resolved Hide resolved
@jakobj
Copy link
Member

jakobj commented Jun 29, 2020

regarding the example: the mutation rate was chosen accordingly to the CGP literature, assuming the implementation is correct, so it should be fine. seems like they all still work, don't they?

@mschmidt87
Copy link
Member

regarding the example: the mutation rate was chosen accordingly to the CGP literature, assuming the implementation is correct, so it should be fine. seems like they all still work, don't they?

I haven't tested the examples. The CI only checks if they crash, not if they convert.

@mschmidt87
Copy link
Member

I think that CI is failing because of flake8 issues. I would recommend to install pre-commit hooks for git to catch such errors locally, @HenrikMettler .

test/test_genome.py Outdated Show resolved Hide resolved
@jakobj
Copy link
Member

jakobj commented Jun 29, 2020

regarding the example: the mutation rate was chosen accordingly to the CGP literature, assuming the implementation is correct, so it should be fine. seems like they all still work, don't they?

I haven't tested the examples. The CI only checks if they crash, not if they convert.

good point! @HenrikMettler please also manually test the examples and check whether they converge properly.

@HenrikMettler
Copy link
Contributor Author

For the examples:

  • all of them stop prior to the max number of generations with values ~ -1e-15 (to -1e-30) -> that this is convergence I assume?
  • example_evo_regression.py: The first only converges after 950 of (max) 1000 generations. Should I increase the max number of allowed generations?

@jakobj
Copy link
Member

jakobj commented Jun 29, 2020

no, i think it's fine, no need to change it as long as it converges within the given number of generations. thanks for checking this :)

[edit]
i think as soon as you've reintroduce the "long loop" in the test i'm fine. travis is also fine, it's just showing an error due to a communication issue between travis and github. before we merge please squash your commits.
[/edit]

@HenrikMettler
Copy link
Contributor Author

the latest commit should clear all the issues. I can't squash though (I think) since i don't have the rights to squash and merge (if I understand this: https://cloudfour.com/thinks/squashing-your-pull-requests/ correctly)

Copy link
Member

@mschmidt87 mschmidt87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for catching this bug and fixing it right away.

@mschmidt87
Copy link
Member

Before we merge, one more (minor) thing: could you make the commit message a bit nicer, i.e., remove the "some more changes" lines etc., and stick to imperative tense and capitalized beginning?

…ation rates for correct number of mutations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken counter for successful mutations in Genome.mutate()
3 participants