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

Extend testing of MuPlusLambda class #171

Merged
merged 1 commit into from
Jul 7, 2020
Merged

Extend testing of MuPlusLambda class #171

merged 1 commit into from
Jul 7, 2020

Conversation

mschmidt87
Copy link
Member

@mschmidt87 mschmidt87 commented Jul 3, 2020 •
edited by jakobj
Loading

closes #166

@mschmidt87 mschmidt87 requested a review from jakobj July 3, 2020 01:30
@mschmidt87 mschmidt87 linked an issue Jul 3, 2020 that may be closed by this pull request
More thorough testing of mu_plus_lambda #166
Closed
@mschmidt87
Copy link
Member Author

Coverage behaves strangely: I wrote explicit tests for both errors raised in the algorithm, but it still reports the 2nd one to be not covered by the tests.

@jakobj jakobj added this to the 0.2.0 milestone Jul 3, 2020
jakobj
jakobj requested changes Jul 3, 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.

wow, awesome, you're fast! 🚀 i've added a few comments that should be a addressed before merging.

further, could you please also implement a test that makes sure we prefer offsprings with equal fitness over their parents? we claim that in the comment before the construction of combined and it's very important for the algorithm to work properly.

could you please also write a simple test for _compute_fitness, i think that's only tested implicit currently.

test/test_ea_mu_plus_lambda.py Outdated Show resolved Hide resolved
test/test_ea_mu_plus_lambda.py Outdated Show resolved Hide resolved
test/test_ea_mu_plus_lambda.py Outdated Show resolved Hide resolved
test/test_ea_mu_plus_lambda.py Outdated Show resolved Hide resolved
test/test_ea_mu_plus_lambda.py Show resolved Hide resolved
test/test_ea_mu_plus_lambda.py Outdated Show resolved Hide resolved
test/test_ea_mu_plus_lambda.py Outdated Show resolved Hide resolved
test/test_ea_mu_plus_lambda.py Outdated Show resolved Hide resolved
test/test_ea_mu_plus_lambda.py Outdated Show resolved Hide resolved
test/test_ea_mu_plus_lambda.py Outdated Show resolved Hide resolved
@mschmidt87
Copy link
Member Author

Thanks for your feedback, I implemented the suggestions and addressed all comments. Please check again @jakobj

@mschmidt87 mschmidt87 requested a review from jakobj July 4, 2020 08:17
jakobj
jakobj requested changes Jul 5, 2020
View reviewed changes
Copy link
Member

@jakobj jakobj left a comment •
edited
Loading

Choose a reason for hiding this comment

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

looks great, just a few minor suggestions.

your inline comments all start with a capital letter, while most of the others in the package start with a lower case one. am i missing something here? shouldn't this somehow be regulated by black?

test/test_ea_mu_plus_lambda.py Outdated Show resolved Hide resolved
test/test_ea_mu_plus_lambda.py Outdated Show resolved Hide resolved
test/test_ea_mu_plus_lambda.py Show resolved Hide resolved
test/test_ea_mu_plus_lambda.py Outdated Show resolved Hide resolved
@mschmidt87
Copy link
Member Author

your inline comments all start with a capital letter, while most of the others in the package start with a lower case one. am i missing something here? shouldn't this somehow be regulated by black?

I am not aware of any policy regarding capitalization of inline comments, neither in pep8 (https://www.python.org/dev/peps/pep-0008/#inline-comments) nor in black.

@mschmidt87
Copy link
Member Author

Thanks @jakobj , I used your suggestions.

@mschmidt87 mschmidt87 requested a review from jakobj July 6, 2020 11:35
jakobj
jakobj approved these changes Jul 6, 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.

awesome, thanks, looks great! 👍

@jakobj
Copy link
Member

jakobj commented Jul 6, 2020

oh one more thing: could you please squash the commits before merging?

@mschmidt87
Extend testing of MuPlusLambda class
8dc4c20 
Add tests for single function of MuPlusLambda class
Co-authored-by: Jakob Jordan <1562742+jakobj@users.noreply.github.com>
@mschmidt87 mschmidt87 merged commit 6705227 into master Jul 7, 2020
@mschmidt87 mschmidt87 deleted the tests/ea_mu_plus_lambda branch July 7, 2020 00:41
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.

More thorough testing of mu_plus_lambda
2 participants
@mschmidt87 @jakobj