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

Review Multiobjective compatibility #91

Open
wants to merge 48 commits into
base: development
Choose a base branch
from
Open

Conversation

DaanZ
Copy link

@DaanZ DaanZ commented May 12, 2020

Formally used to review the changes between the multiobjective branch and the development branch.

Copy link
Author

@DaanZ DaanZ left a comment

Choose a reason for hiding this comment

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

Environment involved in many of the functions might not be the best approach when environment is just a dictionary of one element, if it is more than one, we can also do this from the experiment file instead of pushing this information deeper into the Revolve system.

measure, value = line.strip().split(' ')
behavior_headers.append(measure)
file_summary.write(measure+'\t')
behavior_headers.append('velocity')
Copy link
Author

Choose a reason for hiding this comment

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

Revert Changes to put the criteria in the behavior_desc_robot_1 text file.

@@ -56,8 +67,12 @@ def build_headers(path):
if os.path.isfile(bh_file):
with open(bh_file) as file:
for line in file:
measure, value = line.strip().split(' ')
file_summary.write(value+'\t')
if line != 'None':
Copy link
Author

Choose a reason for hiding this comment

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

Why would the line be "none"?

@@ -87,4 +102,4 @@ def build_headers(path):
if 'body' in file:
id = file.split('.')[0].split('_')[-1]
file_summary.write(gen+'\t'+id+'\n')
file_summary.close()
file_summary.close()
Copy link
Author

Choose a reason for hiding this comment

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

Revert


measures_snapshots_all = sqldf("select * from measures_snapshots_all where fitness IS NOT NULL")

fail_test = sqldf(paste("select method,run,generation,count(*) as c from measures_snapshots_all group by 1,2,3 having c<",gens," order by 4"))
Copy link
Author

Choose a reason for hiding this comment

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

Why is this rearrangement necessary?

pyrevolve/experiment_management.py Show resolved Hide resolved
pyrevolve/experiment_management.py Show resolved Hide resolved
pyrevolve/experiment_management.py Show resolved Hide resolved
pyrevolve/experiment_management.py Show resolved Hide resolved
@@ -46,6 +41,97 @@ def handle_swap(genotype):
return genotype


def handle_addition(genotype, genotype_conf):
Copy link
Author

Choose a reason for hiding this comment

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

Review difference with development function.

for subitem in item:
clause += str(subitem) + ' '

clause_is_true = eval(clause)
Copy link
Member

Choose a reason for hiding this comment

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

Why eval? Please no!
We will never be able to debug any of this

Copy link
Member

@portaloffreedom portaloffreedom left a comment

Choose a reason for hiding this comment

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

There is no urgency to merge this into the main branch and it adds lots of complexity difficult to manage.
I would put this PR on hold for the moment.

# generates clause and rule for each flavor of the letter
for flavor in range(0, conf.max_clauses):

grammar[symbol[0]].append([])
Copy link
Member

Choose a reason for hiding this comment

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

why creating an empty list and then extend it? Is it not easier to just create the correct list in the fist place?


def build_clause(environmental_conditions, logic_operators, max_terms_clause):

clause = []
Copy link
Member

Choose a reason for hiding this comment

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

this creates a list of statements that later is concatenated in a string that is evaluated and assumes some variables present in the context of the function (namely: "hill", "hot" or any other string present in environmental_conditions).

This is so confusing I don't even know where to start.

max_clauses=2,
max_terms_clause=2,
plastic=False,
environmental_conditions=['hill'],
Copy link
Member

Choose a reason for hiding this comment

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

If you want to add a new condition here, there is nowhere written that you also MUST create this variable inside the early_development function of plasticoding.py

And I don't like this way of doing things. You shouldn't touch multiple files all over the place when you need to configure one thing. And you shouldn't touch files outside of your experimental folder when configuring an experiment.

@DaanZ DaanZ self-assigned this May 25, 2020
DaanZ and others added 2 commits May 25, 2020 00:58
add connections of inpurs to oscillators
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.

3 participants