Add decision filter#21
Conversation
…to add-decision-filter
added newline
yyyliu
left a comment
yyyliu
left a comment
There was a problem hiding this comment.
Thanks for the PR! However we may need to rethink the design ... and the change might be pretty drastic.
| os.chdir(cwd) | ||
|
|
||
| class PythonDict(click.Option): | ||
|
|
There was a problem hiding this comment.
Could you add more comments / documentation? In general, we'll need one Docstring per function, ideally describing (1) what the function does, (2) any parameters, (3) the return value. You might only do (1) if the function is simple. Search for "Docstring" to learn more about the format
| os.system(line) | ||
| os.chdir(cwd) | ||
|
|
||
| class PythonDict(click.Option): |
There was a problem hiding this comment.
The name of the class is a bit too general. Maybe indicating it's a custom click command class. Also why not move it to cli.py (if it's only used there? I might be wrong)
| default='') | ||
| def compile(script, out, lang): | ||
| @click.option('--decisions', '-d', cls=PythonDict, default='{}') | ||
| def compile(script, out, lang, decisions): |
There was a problem hiding this comment.
Would it be more flexible if we add this "decisions" option to boba run instead of boba compile? So we only need to generate the entire multiverse once, and we could have the flexibility of running a small subset based on decision values. This will be a very useful feature to add to boba run.
There was a problem hiding this comment.
And the implementation will be much simpler as we'll only need to read and filter the summary.csv to get the correct universe ids. Are there particular reasons that modifying boba compile is better?
There was a problem hiding this comment.
the main issue with making this part of boba run and not boba compile is continuous variables.
Let's say i have a template that has a continuous variable A with range 0..1, and a static variable B with options "red", "green", "blue".
Now lets look at the two different cases, filter on boba run and filter on boba compile.
filter on boba run:
We already ran boba compile and generated the universes. This means there is some number of universes where A is in the range 0..1 and B has values "red", "green", "blue".
Filtering on B is fine: we can easily do boba run -d "{'B': 'red'}", and that would run all of the generated universes that have B = "red".
Filtering on A, however, is problematic. Suppose I do boba run -d "{'A': 0.5}". Per the spec provided by our user, this is legal. A should be able to take on any value in the range 0..1. But in the implementation, this is not the case, as boba compile creates a set of discrete universes via sampling, not infinite universes. Thus, there may not be a generated universe such that A = 0.5.
How could we fix this? Well, we could pick the universe with the closest A value, but that is technically lying to the user since we're not actually testing with A = 0.5. And what if the number of samples is low and there really isn't a 'close enough' A?
filter on boba compile:
Again, filtering on B is trivial: simply generate the universes where B is a particular value.
Filtering on A is also trivial now, since we can simply generate the universes where A is a particular value.
That was my reasoning behind choosing to use boba compile instead of boba run.
There was a problem hiding this comment.
Thanks! The example was helpful and I think the reasoning makes sense. As we discussed in the meeting, besides the review points we'll also need to combine path_filter with the rest, as placeholder vars and blocks aren't allowed to have the same identifier.
| self._warn_size() | ||
| self._code_gen() | ||
|
|
||
| if decisions is not None: |
There was a problem hiding this comment.
perhaps move this chunk of code to a new function in the parser class (or a function in the decision parser class). We want to keep this main function nice and simple. Also, as we've discussed, placeholder variables are not allowed to have the same name as code blocks, so maybe we'll need some revisions related to this.
| elif path_filter is None: | ||
| for idx, p in enumerate(paths): | ||
| self._code_gen_recur(p, 0, '', History(idx, '', decrecords, [])) | ||
| else: |
There was a problem hiding this comment.
Similarly you might want to encapsulate this chunk of code in a function that returns valid_paths. And here you'll only need a line to get the valid paths. We can probably get rid of the if-else block too (if it's possible to pass an empty decrecord to History and it will still work properly). Smaller functions that do simple things are much easier to read and maintain.
| prev_option = d.option | ||
|
|
||
| if prev_idx is not None: | ||
| if prev_option is not None: |
There was a problem hiding this comment.
does prev_option has a fixed type (e.g. string)? We need to make sure a user-defined option will never be None. And if you use == with two options somewhere else, it might not work for types like floating point ... I used index before because it is an integer and we do not have to worry about these.
| default='') | ||
| def compile(script, out, lang): | ||
| @click.option('--decisions', '-d', cls=PythonDict, default='{}') | ||
| def compile(script, out, lang, decisions): |
There was a problem hiding this comment.
Thanks! The example was helpful and I think the reasoning makes sense. As we discussed in the meeting, besides the review points we'll also need to combine path_filter with the rest, as placeholder vars and blocks aren't allowed to have the same identifier.
2 participants
Adds decision filter to
boba compilecommand, and adds test cases for this new functionality.the decision filter is an optional parameter to the
boba compilecommand, specified by usingboba compile --decisions {x}orboba compile -d {x}the
{x}is a string representation of a dictionary containing the decisions to fix, and their values. For example:boba compile -d "{'cutoff': 2.5}"would generate all the universes in the multiverse where the variable
cutoffis equal to 2.5.Furthermore, code paths can be specified by using
path_filterin the dictionary. For example:boba compile -d "{'path_filter' : {'A' : 'iqr'}, 'cutoff' : 2.5}"would generate all the universes where the code path includes the node 'A' with the value 'iqr' for that node.
ValueErroris thrown if the passed in dictionary contains: