-
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
Compare set expression for output to target expression #359
base: master
Are you sure you want to change the base?
Compare set expression for output to target expression #359
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.
great, thanks for working on this! this will prevent much pain. 👀
i've left a few comments that should be addressed before merging.
…onal, add test case
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.
almost done 🚀 left two more comments
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.
great, just some leftover to cleanup from when target_expression
was optional. 👍
afterwards, please squash commits.
ping @HenrikMettler |
Did @HenrikMettler resolve all your comments, @jakobj ? Could you give this another check? |
i think i don't have any more pressing comments, besides squashing commits please and fixing tests :) |
@HenrikMettler could you fix the tests, rebase on master and squash the commits? |
When using
genome.set_expression_for_output
one usually has a target expression that the output node should compile to. Since mistakes in setting the dna can happen easily (eg. by changing the order of primitives) - a built-in check can be useful. This PR implements the comparison usingsympy.parse_expr
and a compilation to sympy of the genome.