-
Notifications
You must be signed in to change notification settings - Fork 11
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
Dev/genome reorder #239
Dev/genome reorder #239
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome, thanks for working on this! 🚀
i've added a few inline comments that should be addressed
also: mypy reports issues, please fix them :) |
Should I add more tests for the auxiliary functions used in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome, this is already much easier to understand, isn't it? 👌
i've added a few more inline comment
3f984f5
to
50f887c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, we're getting there :) some more inline suggestions
50f887c
to
83a591c
Compare
Are you still working on this, @HenrikMettler ? |
Yes, was on holiday last week and this week I have some other priorities. Will probably wrap this up next week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome work! 🥇
i've added some inline comments that should be addressed before merging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work! 🚀
i've left a few more inline comments, i'd say one more round and we're ready to merge.
cgp/ea/mu_plus_lambda.py
Outdated
@@ -25,6 +25,7 @@ def __init__( | |||
n_processes: int = 1, | |||
local_search: Callable[[IndividualBase], None] = lambda combined: None, | |||
k_local_search: Union[int, None] = None, | |||
reorder_genome: bool = True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mschmidt87 any strong feelings about changing this default behaviour? for 1.0 i'd anyway like to use this as the default, so we might as well change it now, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I set this default behavior to False
for now? Otherwise, the tests and examples would require adaptation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, do it :) we can still change it at a later point in time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the update!
a bit more nitpicking and please implement the test that raise exceptions as my inline comment indicates. otherwise this looks awesome!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome stuff! 👍
just one more question and I'd like to hear @mschmidt87's thoughts on changing the default behavior. if the default is changed, i guess we should adopt all tests and examples accordingly.
also, before merging it would be good to rewrite commit history such that each commit applies a logical change.
227dde8
to
b4aff44
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome, great work! 👍
before merging, the history still needs some cleaning up imho. i would suggest organizing this PR in three commits:
- implementation of reorder
- addition of tests
- addition of example
b4aff44
to
d64da42
Compare
Is it ok, if I squash everything in one commit? Due to the previous squashing it is difficult to separate implementation, examples and test development (but I will try to keep this better seperated in future PR's) |
7b20c7b
to
0f44a01
Compare
@jakobj travis found a logic mistake in the example. I fixed it and while doing also adapted some docstrings to make more sense. Could you quickly look at them before merging? |
looks great, thanks! 👍 travis is also happy! please squash the new commit, then it's ready to be merged. 🎉 |
5fff500
to
13ae8b4
Compare
Implements a first solution to #225
Reorders the nodes in the parents genome(s) before creating offspring, without violating any node dependencies, as suggested in Goldman 2015. Works only with
levels_back = n_columns
andn_rows = 1
since possible issue with limitations in levels_back and more than 1 row are not accounted for.Currently I did not add a new example for this functionality but changed
levels_back = n_columns
inexample_minimal.py
and set the boolean for doing reorder to true.Testing currently only covers repeated application of reorder on two examples genomes, while asserting no changes in the resulting sympy expression.