Input File Updates

[gwenchee]

In this PR, I added 2 new input parameters to ROLLO's input file and updated one input parameter.

Added parameters
weight: list. This parameter was added into the algorithm section of the input file, and indicates the weight for each objective as a list.
order: int. This parameter was added into the evaluators section of the input file, and indicates the order of software evaluations. It is indexed by 0

Updated parameters
keep_files: string. This parameter used to be a True, False Boolean. I updated it to be a string input. Accepted strings are none, only_final, all.

[pep8speaks]

Hello @gwenchee! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-03-08 17:08:32 UTC

[yardasol]

Hi @gwenchee, thanks for your contribution. These changes are quite clean! I'm having a bit of trouble understanding them though. Could you elaborate on the big picture purpose/motivation?

Also, I have a few comments:

1. I like that you're using jsonschema to validate your input files. However, I think defining the schema inside the python file takes up quite a bit of space and can distract from the focus on the algorithm of the functions. Consider moving the schema definitions to their own .json files and adding descriptions for each object/parameter as part of the schema.

2. Do you have unit tests for testing that the new input parameters are successfully validated?

• 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, .swp files,
    etc.)?
  • Do all files in the PR belong in the repository?
  • 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?
• 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.

[gwenchee]

Thanks for the review @yardasol. I addressed your comments by updating the docstrings.

Hi @gwenchee, thanks for your contribution. These changes are quite clean! I'm having a bit of trouble understanding them though. Could you elaborate on the big picture purpose/motivation?

In the big picture, the weights parameter allows the user to indicate the weighting of each objective which is interesting for multi-objective simulations. The order parameter is used to ensure that the evaluators are run in the correct order, and also as a sanity check for the user to feel more in control of the order of evaluations. keep_files parameter used to be a boolean, but I added the only_final option for users that lack file space, but still want the option to see their evaluators files for the final generation.

Also, I have a few comments:

1. I like that you're using jsonschema to validate your input files. However, I think defining the schema inside the python file takes up quite a bit of space and can distract from the focus on the algorithm of the functions. Consider moving the schema definitions to their own .json files and adding descriptions for each object/parameter as part of the schema.

I disagree with this. The schema validation happens in input_validation.py which is it's own contained file, not distracting from algorithm.py and evaluation.py where the actual logic takes place.

2. Do you have unit tests for testing that the new input parameters are successfully validated?
   I don't I added an issue for this. unit test for input_validation.py #29

[gwenchee]

@yardasol bump

[yardasol]

Thanks for addressing my comments. I'm going to approve.

I want to clarify a bit my comment about the JSON Schema: when I said that having the schema defined in input_validation.py distracts from the focus of the algorithm, I was specifically referring to the functions in input_validation.py, since you seem to have a lot of infrastructure set up in that particular file. I was not referencing any other files in the repo. I do agree with your point that since everything in that file is related to input validation, it's not a huge issue to define the JSON schema in there.

Another thought I just had is that moving the JSON schema for the main ROLLO input file to its own .json file may make the input file structure a bit more discoverable than having it defined in input_validation.py. I think discoverability is important since input files are a user-facing component.

[gwenchee]

@yardasol Yeah i totally see where you're coming from about the input_validation. I'll keep that in mind