First major ROLLO PR with source code, tests, and one example problem

[gwenchee]

This PR is huge because it contains all the source code and tests for v1.0 of ROLLO.

PR overview:
/docs/: barebones documentation (I will update this to provide examples and more explanation in a future PR)
/examples/: a single FHR slab ROLLO optimization problem
/rollo/: ROLLO's source code
/tests/: ROLLO unit test files

Seeing that this PR is so large, perhaps different people may review different parts of the PR.

Here's a visualization and table describing ROLLO's architecture:

[abachma2]

I'll take number 3

[abachma2]

The part 3 files look good to me. Some comments to make things a little more clear and readable. Nice work!

[gwenchee]

Thanks for the review @abachma2, I have addressed your comments.

[abachma2]

Thanks for making the changes! Looks great Gwen!

[gwenchee]

I deleted the /docs/ directory from this branch and will make a separate PR for documentation. There are no other changes.

[smpark7]

Here's my review. Looks good and mostly clean so far

[gwenchee]

Thanks @smpark7!

[samgdotson]

I haven't run the examples yet. I thought the names of some of the classes was kind of confusing. The hierarchy and the relationship between the classes is not obvious, to me. It's also not obvious what some of your tests are doing. There are a lot of integration tests and not a lot of unit tests.
What happens if someone writes an operator in a slighty different way than is present in the dictionary? This kind of error isn't handled and could become a headache for users.

You've obviously put a lot of thought and effort into this and I don't want to get in the way of the first big release so I'm not requesting changes. However, some of the items I mentioned could become separate issues that could be addressed later.

[samgdotson]

Suggested change

	"Failed to import mpi4py. (Only important for parallel method: mpi_evals)"
	"Failed to import mpi4py. Multithreading disabled."

[samgdotson]

I'm not sure my suggestion makes me happy, either. Basically, the warning could be more descriptive.

[gwenchee]

I have 2 methods for parallelization that I will outline in the docs: multiprocessing and mpi_evals. mpi_evals is a unique implementation that is helpful when using a supercomputer.

[samgdotson]

So then there are two tasks that need to be done:

1. Add an explanation differentiating two types of parallelization in the docs.
2. Make the warning clearer.

Maybe add "reverting to multiprocessing" or something to that effect.

[gwenchee]

Yup 1. is definitely being done. Yeah I agree about 2), let me make an issue for it: #9

[samgdotson]

Is Algorithm really the best name for this? This sounds like a container for simulation info. Maybe Simulation_Info or something similar, might be better.

[samgdotson]

If this runs (some might say evaluates... or executes... ) the algorithm, what is the purpose of the Evaluation class?

[gwenchee]

See #6 (comment) to understand why I have an Executor class, an Algorithm class, and also an Evaluation class.

[samgdotson]

I'm confused by the documentation, then. Reading the docstrings from each class does not give a sense for the overall organization of the rollo package.

class Evaluation:    
    """
    Holds functions that generate and execute the evaluation solver's scripts.

    ..."""

class Algorithm(object):    
    """
    Builds and runs Genetic Algorithms.    
    This class holds functions to generate a generic genetic algorithm.

    ..."""

class Executor(object):    
    """
    Executes rollo simulation from start to finish.    
    Instances of this class can be used to perform a rollo run.
    ..."""

I'm not trying to be pedantic about this. It really isn't clear to me from reading the docstrings what each class should be used for. You have great explanations in a LaTeX table in your comment #6, those explanations should be inside the doc strings for your classes.

[gwenchee]

Okay i hear you on that, I'll add more description for those classes.

[gwenchee]

I just added more descriptions. I appreciate the pedantic-ness, it helps keep me accountable with my documentation. :)

[samgdotson]

There should be docstrings explaining what the test does.

[gwenchee]

Every unit test is named after the function in the source code that it tests. So, I think it is clear what it is doing. For example, the function test_apply_mating_operator in test_algorithm.py tests the function apply_mating_operator in algorithm.py.

[samgdotson]

For which cases? Only one case? The docstring doesn't need to be long, just enough to indicate what is being tested and what case it covers. For example:

def test_grid_optimize_1():    
"""    Case 1: Too many variables given    
"""    
    with pytest.raises(AssertionError):        
        loss = grid_optimizer(X_in.T,                              
        params,                              
        args=['trainlen', 'rho', 'noise'],                              
        xset=trainingLengths,                              
        verbose=False,                              
        visualize=False)
    return

Of course, writing tests takes time. I don't want to get in the way of progress so you may consider creating an issue for this. At the very least, you could number each test you have test_func_1 and add a docstring as shown in my example s.t. future tests can be added so that it's obvious what each test does and what edge/corner cases still need to be covered.
I think it's fine to not have a complete suite of tests in this first major PR. In fact, it provides a great opportunity for undergrads to practice writing tests. This has three benefits:

1. Undergrads get experience coding
2. You get a more robust software package
3. Rollo gets an expanded user base

In sum: Number your tests, add docstrings.

[gwenchee]

Could you share the resource you used that suggests that tests should be numbered? I've not come across this style of test naming (I've been referring to openmc and numpy). Sure, I can make an issue to expand testing: #10

[samgdotson]

Looking at my own test doc string, I can see that it could be clearer also!
"Which variables? Oh, the 'args' parameter."

[samgdotson]

The numbering is probably just a "me" thing. Often a test will be something like: test_func_case followed by a docstring elaborating on the case. I couldn't come up with succinct enough test names so I tend to just number them.

[gwenchee]

Oh I see, in that case, I think i'm comfortable with my naming convention since it pairs with the function in the source file.

[gwenchee]

But perhaps, I'll consider expanding to numbering as I or an undergrad addresses #10

[robfairh]

I think Sam's suggestion is sensible. If in the future you wanted to add more tests, you just add them, without changing the names. Also, in some cases, you could have interior, edge, and cases (see Katy's book). Having that either in the name of the function, with numbers, or the docstrings would be helpful.

[robfairh]

It is also a good idea to keep everything consistent. If you don't have any docstring for your tests, keep it that way, because changing it at this point might be too cumbersome. Also you could have an undergrad to do that, add docstrings to all the tests.

[samgdotson]

same thing as before. What does this test do?

[gwenchee]

See above comment.

[samgdotson]

[samgdotson]

The purpose of this class and the Algorithm class seems to have some overlap.

It's not totally clear what the distinction is between these two classes.

[gwenchee]

See #6 (comment) to understand why I have an Algorithm class, and BackEnd Class

[samgdotson]

What if someone accidentally enters != rather than !=? Is there a test for this (with an errant space)?

[gwenchee]

Yup the validation occurs in input_validation.py on line 331: https://github.com/gwenchee/rollo/blob/c56dd37fea468c9a3b61526899f1a758c1aea410/rollo/input_validation.py#L331

[samgdotson]

Great, you have some tests for this as well. Although, my comment regarding cases and docstrings still applies.

[gwenchee]

Thanks for the review @samgdotson.

I haven't run the examples yet. I thought the names of some of the classes was kind of confusing. The hierarchy and the relationship between the classes is not obvious, to me.

An overview of the class relationships are outlined in the comment I included at the top of this PR: #6 (comment)

It's also not obvious what some of your tests are doing. There are a lot of integration tests and not a lot of unit tests.

Every single test is actually a unit test. As mentioned in the above comments, every unit test is named after the function in the source code that it tests. For example, the function test_apply_mating_operator in test_algorithm.py tests the function apply_mating_operator in algorithm.py.

[samgdotson]

Based on the strucure and design I can tell a lot of thought went into this project. It is well done. The changes I'm requesting are primarily for clarity.

1. Class docstrings should have enough information to differentiate tasks among them.
2. Tests should have a doc string and a case number.
3. There should be more tests. I would be satisfied with an open issue on this point.

[gwenchee]

Thanks for the review @samgdotson .

1. I've addressed this with updated docstrings.
2. As for this one, could you share the resource you used that suggests that tests should be numbered? I've not come across this style of test naming (I've been referring to openmc and numpy).
3. See Expand unit testing to cover more cases.  #10

[gwenchee]

If it's all good, can we merge? @samgdotson

[gwenchee]

Hi @samgdotson @smpark7 @abachma2 could someone merge this?