Update defaults #15
Update defaults #15
Conversation
gwenchee
added
the
Type:Bug
There was a problem hiding this comment.
-[x] Read the PR description
- Read through all the changes, considering the following questions.
- Is there anything you can praise about this PR? Start with
praise. - Are variable names brief but descriptive?
- Are new/changed functions no longer than a paragraph?
- Do all function parameters have default values where appropriate?
- Is the code clear and clean? (see Robert C. Martin's
Clean Code) - Is there enough documentation?
- Does the programming style meet the requirements of the
repository (PEP8 for python,
google C++ style guide, etc.) - If a new feature has been added, or a bug fixed, has a test been
added to confirm good behavior? - Does the test actually test the new/changed functionality?
- Does the test successfully test edge cases?
- Does the test successfully test corner cases?
- If the repository has continuous integration: does the PR pass
the tests? - If there is no continuous integration, check out the branch
locally, build, and run the tests. - Do the tests pass on your machine?
- Is the PR free of random cruft (built files,
.swpfiles,
etc.)? - Do all files in the PR belong in the repository?
- If the PR deletes files, is this appropriate?
- If the PR adds files or data, are these new files compatible
with the repository license? - Make a review, leaving kind comments and suggesting changes
where needed (to resolve the above). - Has the author resolved all of the comments and suggestions
in your review?
- Is there anything you can praise about this PR? Start with
- When you approve of the PR, merge and close it.
- Does this PR close an issue? If so, be sure to descriptively
close this issue when the PR is merged - Thank the author for their contribution.
Hi @gwenchee, thanks for your PR. Good job updating the code layout to make it more readable. I also think the extra input validation will help in the future. I was able to checkout your local changes and all the tests passed! I did get lots of warnings, although that may have more to do with my machine environment. Nevertheless, I'll post them down below.
Some general feedback
- The
validatefunction is a bit long. There's a lot of repeated snippets that could be condensed into a helper function. Not a critical change, but it might help readability. - Some functions have variable names that could be confusing to discuss. I've already left some comments on that in my review, but as another example, looking at the function
default_check, one of the variables is calledvariable. I would strongly recommend adding a quantifier or changing the name of this variable to something more descriptive.variable_namewould be a good option. - Some of your docstrings have ambiguity in the parameter descriptions (e.g. in
default_check, the description forvariableis justvariable name. Consider adding a bit more context, likeName of input file variable to check. You should do this in your own words of course)
Overall I think this a good PR. I'm going to approve it within the scope of resolving issue #13. I think some of the points I brought up should be addressed/turned into an issue though.
Here's my test output:
============================================================ test session starts =============================================================
platform linux -- Python 3.7.10, pytest-6.2.5, py-1.10.0, pluggy-1.0.0
rootdir: /home/ooblack/projects/rollo
plugins: anyio-3.3.2, cov-2.12.1
collected 31 items
test_algorithm.py ...... [ 19%]
test_backend.py ..... [ 35%]
test_constraints.py ... [ 45%]
test_evaluation.py .... [ 58%]
test_executor.py .... [ 70%]
test_openmc_evaluation.py . [ 74%]
test_special_variables.py .. [ 80%]
test_toolbox_generator.py ...... [100%]
======================================================================================== warnings summary ========================================================================================
../../../anaconda3/envs/rollo-env/lib/python3.7/site-packages/rollo/algorithm.py:14
/home/ooblack/anaconda3/envs/rollo-env/lib/python3.7/site-packages/rollo/algorithm.py:14: UserWarning: Failed to import mpi4py. (Only important for parallel method: mpi_evals)
"Failed to import mpi4py. (Only important for parallel method: mpi_evals)"
../../../anaconda3/envs/rollo-env/lib/python3.7/site-packages/past/builtins/misc.py:45
/home/ooblack/anaconda3/envs/rollo-env/lib/python3.7/site-packages/past/builtins/misc.py:45: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
from imp import reload
../../../anaconda3/envs/rollo-env/lib/python3.7/site-packages/deap/creator.py:141: 1 warning
tests/test_constraints.py: 1 warning
tests/test_evaluation.py: 1 warning
tests/test_executor.py: 2 warnings
tests/test_toolbox_generator.py: 5 warnings
/home/ooblack/anaconda3/envs/rollo-env/lib/python3.7/site-packages/deap/creator.py:141: RuntimeWarning: A class named 'obj' has already been created and it will be overwritten. Consider deleting previous creation of that class or rename it.
RuntimeWarning)
../../../anaconda3/envs/rollo-env/lib/python3.7/site-packages/deap/creator.py:141: 1 warning
tests/test_constraints.py: 1 warning
tests/test_evaluation.py: 1 warning
tests/test_executor.py: 2 warnings
tests/test_toolbox_generator.py: 5 warnings
/home/ooblack/anaconda3/envs/rollo-env/lib/python3.7/site-packages/deap/creator.py:141: RuntimeWarning: A class named 'Ind' has already been created and it will be overwritten. Consider deleting previous creation of that class or rename it.
RuntimeWarning)
tests/test_algorithm.py::test_generate
/home/ooblack/anaconda3/envs/rollo-env/lib/python3.7/site-packages/rollo/constraints.py:128: UserWarning: 3 out of 10 inds were constrained
" inds were constrained"
tests/test_algorithm.py::test_generate
tests/test_algorithm.py::test_apply_algorithm_ngen
/home/ooblack/anaconda3/envs/rollo-env/lib/python3.7/site-packages/rollo/constraints.py:128: UserWarning: 1 out of 10 inds were constrained
" inds were constrained"
tests/test_algorithm.py::test_generate
tests/test_algorithm.py::test_apply_algorithm_ngen
/home/ooblack/anaconda3/envs/rollo-env/lib/python3.7/site-packages/rollo/constraints.py:128: UserWarning: 2 out of 10 inds were constrained
" inds were constrained"
tests/test_algorithm.py::test_generate
/home/ooblack/anaconda3/envs/rollo-env/lib/python3.7/site-packages/rollo/constraints.py:128: UserWarning: 5 out of 10 inds were constrained
" inds were constrained"
tests/test_algorithm.py::test_initialize_pop
/home/ooblack/anaconda3/envs/rollo-env/lib/python3.7/site-packages/rollo/constraints.py:128: UserWarning: 1 out of 5 inds were constrained
" inds were constrained"
tests/test_algorithm.py::test_apply_selection_operator
/home/ooblack/anaconda3/envs/rollo-env/lib/python3.7/site-packages/rollo/constraints.py:128: UserWarning: 6 out of 10 inds were constrained
" inds were constrained"
tests/test_constraints.py::test_apply_constraints
/home/ooblack/anaconda3/envs/rollo-env/lib/python3.7/site-packages/rollo/constraints.py:128: UserWarning: 3 out of 5 inds were constrained
" inds were constrained"
-- Docs: https://docs.pytest.org/en/stable/warnings.html
================================================================================ 31 passed, 31 warnings in 13.56s ================================================================================
| @@ -0,0 +1,124 @@ | |||
| { | |||
There was a problem hiding this comment.
Suggestion: Use some markdown cells to guide human readers through what it is you are doing. In my own research this has been a lifesaver when coming back to a project that I've left stale for a while, or when looping a new participant into a project.
There was a problem hiding this comment.
I think this notebook is quite readable since there are very few actions. I'll definitely add some markdown cells for more complex notebooks in the future.
| @@ -20,13 +20,6 @@ | |||
| "pop_size": 4, | |||
| "generations": 10, | |||
| "mutation_probability": 0.2374127402121101, | |||
| "mating_probability": 0.4699131568275016, | |||
There was a problem hiding this comment.
Can you explain these deletions?
There was a problem hiding this comment.
From this PR, ROLLO will add default values for these parameters.
| " since the outputs: " + | ||
| str(which_strings) + | ||
| " are not inputs or pre-defined outputs." | ||
| "<Input Validation Error>" |
There was a problem hiding this comment.
Ooh yeah the way these plus signs are is much better :)
| str(accepted_variables) + | ||
| " not variable: " + | ||
| variable | ||
| "<Input Validation Error> variable: " |
| input_algorithm = self.default_check(input_algorithm, "generations", 10) | ||
| input_algorithm = self.default_check( | ||
| input_algorithm, "selection_operator", {"operator": "selBest", "inds": 1} | ||
| input_algorithm, "mutation_probability", 0.23 |
There was a problem hiding this comment.
Sanity check: You're adding more checks for the input file, yes?
There was a problem hiding this comment.
What do you mean? All the checks are in the input_validation.py file.
| missing. | ||
| ROLLO input file to ensure the user defined all key parameters. If the | ||
| user did not, ROLLO raises an exception to tell the user which | ||
| parameters are missing. | ||
|
|
||
| Attributes | ||
| ---------- |
There was a problem hiding this comment.
Naming the variable for this class as input could be confusing for folks reading the documentation. Consider adding a quantifier to differentiate this variable from the general concept of an input. Off the top of my head, rollo_input seems like it could be a good option.
There was a problem hiding this comment.
I'll address this in #20
| @@ -33,6 +33,7 @@ def add_all_defaults(self, input_dict): | |||
| input file dict with additional missing default inputs | |||
|
|
|||
| """ | |||
| input_dict = self.input.copy() | |||
There was a problem hiding this comment.
If you go ahead and rename the class variable input, perhaps think about if you need to change the variable name for input_dict as well. That would go for all the other files that also use input_dict. I personally don't think this change is strictly necessary, but maybe you'll find it useful.
There was a problem hiding this comment.
I'll address this in #20
This PR addresses #13 and fixes a bug in ROLLO's input validation. The bug is caused by the default values being added after validation, resulting in an error being thrown due to the validation occurring on an incomplete input file without default values.