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.

Sign up for GitHub

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

Jump to bottom

Add test for randomize_genome function of individual classes #190

Merged
merged 2 commits into from
Jul 20, 2020
Merged

Add test for randomize_genome function of individual classes #190

merged 2 commits into from
Jul 20, 2020

Conversation

mschmidt87
Copy link
Member

@mschmidt87 mschmidt87 commented Jul 15, 2020 •
edited
Loading

Adds a simple test for the randomize_genome function of the individual classes. All it tests is that the randomize_genome function does indeed change the dna.

Closes #186

@mschmidt87 mschmidt87 added the test suite Making sure it's working label Jul 15, 2020
@mschmidt87 mschmidt87 requested a review from jakobj July 15, 2020 12:31
@jakobj jakobj added this to the 0.2.0 milestone Jul 15, 2020
jakobj
jakobj reviewed Jul 15, 2020
View reviewed changes
Copy link
Member

@jakobj jakobj left a comment

Choose a reason for hiding this comment

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

yes! more tests! :) i am wondering however, whether this would not better fit into test_genome the individuals are just forwarding everything to the genome aren't they? furthermore, from a quick grep it seems like randomize_genome is never used?! is this possible? and if so, shouldn't we rather remove it to keep confusion levels low?

test/test_individual.py Outdated Show resolved Hide resolved
@jakobj jakobj removed this from the 0.2.0 milestone Jul 15, 2020
@jakobj
Copy link
Member

jakobj commented Jul 15, 2020 •
edited
Loading

oh and i'm confused: how does this "close" itself? ;)

@mschmidt87
Copy link
Member Author

mschmidt87 commented Jul 15, 2020 •
edited
Loading

oh and i'm confused: how does this "close" itself? ;)

haha, fixed.

@mschmidt87
Copy link
Member Author

i am wondering however, whether this would not better fit into test_genome the individuals are just forwarding everything to the genome aren't they? furthermore, from a quick grep it seems like randomize_genome is never used?! is this possible? and if so, shouldn't we rather remove it to keep confusion levels low?

My main motivation here was to not leave the code untested, but yeah, this function is pretty redundant and if it not used, we can also remove it altogether, I agree. If that is our decision, we can close this PR and I'll open another one tomorrow, removing this functionality from the classes.

@mschmidt87
Copy link
Member Author

On 2nd thought, I think that we should instead of removing the function, rather use it in the Population in the generate_random_individual. Currently, we are doing a rather convoluted type check which is necessary because we call the genome.randomize() method. If instead, we call individual.randomize_genome(), this should become much easier.

@mschmidt87 mschmidt87 requested a review from jakobj July 16, 2020 12:56
@mschmidt87
Copy link
Member Author

I implemented the change in Population

@mschmidt87
Copy link
Member Author

Please take another look @jakobj

@jakobj jakobj removed the test suite Making sure it's working label Jul 19, 2020
jakobj
jakobj requested changes Jul 19, 2020
View reviewed changes
Copy link
Member

@jakobj jakobj left a comment

Choose a reason for hiding this comment

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

just one small suggestion that would avoid an import of copy, otherwise this look good!

test/test_individual.py Outdated Show resolved Hide resolved
@jakobj jakobj added this to the 0.2.0 milestone Jul 19, 2020
@mschmidt87 mschmidt87 force-pushed the test/individual_randomize_genome branch from 467fb9e to a43c223 Compare July 20, 2020 03:40
@mschmidt87
Copy link
Member Author

Yep, thanks, I had forgotten about that, fixed it now and already squashed commits to 2 commits. @jakobj

jakobj
jakobj approved these changes Jul 20, 2020
View reviewed changes
Copy link
Member

@jakobj jakobj left a comment

Choose a reason for hiding this comment

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

looks great! 👍 and merging

@jakobj jakobj merged commit 9ff3cac into master Jul 20, 2020
@jakobj jakobj deleted the test/individual_randomize_genome branch July 20, 2020 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakobj jakobj jakobj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.2.0
Development

Successfully merging this pull request may close these issues.

Add test for randomize_genome of individual classes
2 participants
@mschmidt87 @jakobj