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

Permissible values per gene in initialization of genome #201

Conversation

HenrikMettler
Copy link
Contributor

@HenrikMettler HenrikMettler commented Jul 21, 2020

Determine permissible values per gene in initialization of genome. Addresses #196.
I was not sure which of the determine_permissible_value_xxx functions should be private. And please tell me whether you think testing with the permissible values for a single genome is sufficient.

@HenrikMettler
Copy link
Contributor Author

Looking at the discussion in #158 i realized that permissible_values_per_gene should rather be a numpy array than a list (and therefore permissible_values a list of numpy array. I will address this with a new commit.

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! Just two change requests.

cgp/genome.py Outdated Show resolved Hide resolved
test/test_genome.py Outdated Show resolved Hide resolved
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.

this looks great, nice work @HenrikMettler! 👌

i've added a few comments that should be addressed, mostly regarding style.

cgp/genome.py Outdated Show resolved Hide resolved
cgp/genome.py Outdated Show resolved Hide resolved
cgp/genome.py Outdated Show resolved Hide resolved
cgp/genome.py Outdated Show resolved Hide resolved
cgp/genome.py Outdated Show resolved Hide resolved
cgp/genome.py Outdated Show resolved Hide resolved
cgp/genome.py Outdated Show resolved Hide resolved
cgp/genome.py Outdated Show resolved Hide resolved
cgp/genome.py Show resolved Hide resolved
test/test_genome.py Show resolved Hide resolved
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.

Just some requests regarding the type hints.

cgp/genome.py Outdated Show resolved Hide resolved
cgp/genome.py Outdated Show resolved Hide resolved
cgp/genome.py Outdated Show resolved Hide resolved
cgp/genome.py Outdated Show resolved Hide resolved
cgp/genome.py Outdated Show resolved Hide resolved
cgp/genome.py Outdated Show resolved Hide resolved
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.

great, just three more comments :)

cgp/genome.py Outdated Show resolved Hide resolved
cgp/genome.py Outdated Show resolved Hide resolved
cgp/genome.py Outdated Show resolved Hide resolved
cgp/genome.py Outdated Show resolved Hide resolved
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.

excellent work 👍

could you please squash the commits into one before we merge this?

Add new test for permissible gene values. Remove test for alternative_permissible_values since the functions were removed.
@HenrikMettler
Copy link
Contributor Author

done @jakobj

@jakobj
Copy link
Member

jakobj commented Jul 29, 2020

thanks! and merging :)

@jakobj jakobj merged commit 1321b35 into Happy-Algorithms-League:master Jul 29, 2020
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.

Investigate moving determine_alternative_permissible_values function for genes to __init__ of Genome.
3 participants