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

Recorder #27

Merged
merged 40 commits into from
Apr 13, 2017
Merged

Recorder #27

merged 40 commits into from
Apr 13, 2017

Conversation

vevenom
Copy link
Contributor

@vevenom vevenom commented Apr 5, 2017

No description provided.

@vevenom vevenom requested a review from anandtrex April 6, 2017 14:03
@@ -0,0 +1,101 @@
import os
Copy link
Contributor

Choose a reason for hiding this comment

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

So this file has to be removed, and the recorder added to all remaining bin/ltl-* files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done but I'm not sure I understand how to call the other binaries as I'm getting error:
NotStartedProperly("SCOOP was not started properly.\nBe sure to start your program with the '-m scoop' parameter. You can find further information in the documentation.",)

How do i use the "-m scoop" parameter? I added recorder to bin/ltl-* files but cannot test because of this

function_name = self.function_names[ind]
return function, function_name

def __create_rastrigin__(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Double underscore is reserved for system methods and deliberate name mangling. Please use single under-score, prefix-only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


class BenchmarkedFunctions:
def __init__(self):
self.function_names = ["Rastrigin2d", "Chasm", "Shekel2d", "Michalewicz2d", "3Gaussians2d"]
Copy link
Contributor

Choose a reason for hiding this comment

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

We definitely need all the functions here, especially multi-dimensional versions of these functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will need some time to do this

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can add this slowly. It doesn't have to be with this pull request.

class BenchmarkedFunctions:
def __init__(self):
self.function_names = ["Rastrigin2d", "Chasm", "Shekel2d", "Michalewicz2d", "3Gaussians2d"]
self.functions = [self.__create_rastrigin__, self.__create_chasm__, self.__create_shekel__,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just use a list of tuples? Your get_function_by_index will be even simpler then:

    def get_function_by_index(self, ind):
         return function_name_map[ind]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -36,9 +36,15 @@ def __init__(self, fg_params, dims=2, bound=None, noise=False, mu=0., sigma=0.01
self.gen_functions = []
# The class name of the parameter named tuple indexes the actual function class,
# which is initialized using the given param and dims
self.description = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't generate string here. You should use Jinja for this part too.
Why not just pass it as a dict all the way to the recorder and plug it into the jinja template you have?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. But now I need a way to record all optimizee parameters. Dimensionality and noise is fine but I had to add names and params of the involved functions as the member attributes of the FunctionGenerator class although I only need this for the recorder

@@ -0,0 +1,108 @@
from git import Repo
import datetime
from jinja2 import Environment, FileSystemLoader
Copy link
Contributor

Choose a reason for hiding this comment

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

Should add jinja2 and PythonGit to requirements.txt (with correct pypi package names)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

self.runtime = self.end_time - self.start_time
self.__parse_md__()

def __parse_md__(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto comment about double underscores.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

def __init__(self, environment,
optimizee_id, optimizee_name, optimizee_description, optimizer_name, optimizer_parameters):
self.record_flag, self.username, self.description = self.__process_args__()
self.environment = environment
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not pass the trajectory directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

template = env.get_template("md-template.jinja")
with open(fname, 'w') as f:
rendered_data = template.render(context)
f.write(rendered_data)
Copy link
Contributor

Choose a reason for hiding this comment

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

You should print a message here to logger.info saying the details have been recorded in this particular file in the particular path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

…ocess list of namedtuples directly; Add recorder functionality to bin files; Removed ltl-recfun-sa.py as it was only for testing;
Copy link
Contributor

@anandtrex anandtrex left a comment

Choose a reason for hiding this comment

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

Two very minor comments. Otherwise looks good. Will merge in once the two are taken care of.

# Add Recorder
recorder = Recorder(trajectory=traj, optimizee_id=function_id,
optimizee_name=fg_name, optimizee_parameters=fg_instance,
optimizer_name=parameters.__class__.__name__, optimizer_parameters=parameters)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't parameters.__class__.__name__ give the class name of the Optimizer parameters named tuple? Should this be optimizer.__class__.__name__?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -53,12 +56,18 @@ def main():
# Get the trajectory from the environment
traj = env.trajectory

function_id = 4
bench_functs = BenchmarkedFunctions(noise=True)
fg_name, fg_instance = bench_functs.get_function_by_index(4)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be fg_params instead of fg_instance .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@vevenom
Copy link
Contributor Author

vevenom commented Apr 11, 2017

I also added quickly some benchmark functions and also multidimensionals. This should server as the basis for the future benchmark functions. For some the parameters need to change in order to have nice 10d representation so I did not add those. Maybe you should rerun test on ...-ce and ...-face bin files as I am not sure how to run them with right flags

@anandtrex
Copy link
Contributor

Ok.
Sorry I just remembered one more thing you need to do before a merge: Can you add a section to the documentation (doc/intro.rst) describing how to use the recorder?

@anandtrex
Copy link
Contributor

anandtrex commented Apr 11, 2017 via email

…r in intro.rst; Added handling of class parameters for optimizee function description and parameters of the optimizers
@vevenom
Copy link
Contributor Author

vevenom commented Apr 12, 2017

Ok, that should be it. Added the functionality for ce and face and tested it. There the parameters of optimizer can be instance of some class so I added a handle for it. Added a section in intro.rst

@@ -19,7 +19,7 @@

def main():
name = 'LTL-FUN-CE'
root_dir_path = None # CHANGE THIS to the directory where your simulation results are contained
root_dir_path = "/home/sinisa/ltlresults/" # CHANGE THIS to the directory where your simulation results are contained
Copy link
Contributor

Choose a reason for hiding this comment

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

:-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yay, forgot that. Fixed :)

@anandtrex anandtrex merged commit 7134723 into master Apr 13, 2017
@anandtrex anandtrex deleted the recorder branch April 13, 2017 11:58
@anandtrex
Copy link
Contributor

Merged! I did a squash merge, so you might want to continue development off master and not continue on the recorder branch.

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.

2 participants