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

Check tournament_size <= n_parents in hl_api #268

Merged

Conversation

HenrikMettler
Copy link
Contributor

Makes the sanity check that the tournament size is smaller or equal to the number of parents in a population. Closes #246
Note that this PR is not ready for merging - due to the sanity checks various examples now raise errors. (Will adapt if the concept is approved)

@HenrikMettler HenrikMettler added this to the 0.3.0 milestone Jan 5, 2021
@HenrikMettler HenrikMettler marked this pull request as draft January 5, 2021 14:49
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.

thanks for working on this!

however, i would strongly suggest to have such checks not only in the high level API, but deeper in the library. i think it makes most sense in mu_plus_lambda._create_offspring_population before doing anything else. there you have all information necessary to perform the check.

@coveralls
Copy link
Collaborator

coveralls commented Jan 5, 2021

Coverage Status

Coverage decreased (-0.1%) to 94.957% when pulling 811df3e on HenrikMettler:enh/tournament_size into b6d7942 on Happy-Algorithms-League:master.

@HenrikMettler
Copy link
Contributor Author

Note to myself: Address #271 within this PR

@HenrikMettler HenrikMettler linked an issue Jan 5, 2021 that may be closed by this pull request
@HenrikMettler
Copy link
Contributor Author

however, i would strongly suggest to have such checks not only in the high level API, but deeper in the library. i think it makes most sense in mu_plus_lambda._create_offspring_population before doing anything else. there you have all information necessary to perform the check.

I can move it there, but the downside is, that mu_plus_lambda._create_offspring_population is called within mu_plus_lambda.step -> the sanity check will be done at every time step which seems unreasonable to me.

@jakobj
Copy link
Member

jakobj commented Jan 6, 2021

however, i would strongly suggest to have such checks not only in the high level API, but deeper in the library. i think it makes most sense in mu_plus_lambda._create_offspring_population before doing anything else. there you have all information necessary to perform the check.

I can move it there, but the downside is, that mu_plus_lambda._create_offspring_population is called within mu_plus_lambda.step -> the sanity check will be done at every time step which seems unreasonable to me.

indeed, the lower you go in the code the more often you will evaluate such checks. however, in terms of runtime we will not see any effect (+1 if per generation) while making the code more solid. so while in general i agree with your concern, in this case i think it's the best option.

@HenrikMettler HenrikMettler marked this pull request as ready for review January 6, 2021 15:13
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! 👍

let's wait for travis to be happy before merging.

@jakobj jakobj merged commit 76ed211 into Happy-Algorithms-League:master Jan 6, 2021
@mschmidt87 mschmidt87 added the maintenance Fighting the second law label May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Fighting the second law
Projects
None yet
4 participants