Initial implementation of the comparison class

[pratikunterwegs]

This PR includes an initial implementation of the comparison class discussed in #4. This includes a number of internal changes and updates to how scenario objects are structured, but with only minor departures from #3. A number of helper functions have been added, some of which are already discussed in #5 (which is a non-exhaustive list).

A minimal vignette shows some of this functionality, and for now focuses on scenario objects with a very brief introduction to comparison objects; this is WIP #16.

1. Implementation of the comparison class, with helper functions to get scenario names, get the baseline scenario name, and set new scenario names and a new baseline scenario: fixes Function to change baseline scenario in a comparison #13.
2. Functions to check whether two scenarios are comparable, based on their model function, and elements of their model parameter and extra information lists. The elements used to match scenarios on are passed as match_variables, with the option to match only parameter or information names, or to match names and values. This function will be developed further for use within comparisons to filter for scenarios that are comparable to the baseline.
3. Functions to add extra information to scenario objects: fixes Function to add information to scenarios #14.
4. Full code coverage.

A convenient way to review these changes is to check out this branch, and build the {pkgdown} website, which should render the documentation and initial vignette neatly.

[joshwlambert]

Overall really nice work. The code is clean and easily understandable. It's the first time I've reviewed the {scenarios} package as part of a pull request so some of my comments are specific to the changes within this PR, while others are general comments on the package.

I did not find any methodological issues or bugs. Most of my comments are subjective so feel free to incorporate or ignore.

General package comments:

• Package version is currently 0.1.0, I would change this to 0.0.0.9000 to show that the package is still in early development and has not had a formal release.
• Package currently depends on a commit-specific install of {epidemics}. No need to change, just making you aware in case you wanted to update.

Vignette:

• The vignette reads well and has informative examples. I did not find any faults with the text or code examples.
• In the vignette their is a citation of @mossong2008 but there is no .bib file in the package (that I could find), and therefore the reference didn't render.

Pkgdown:

• I get two logos on the pkgdown main page when building the site locally. Not sure if this will remain when the site is deployed online.

Functions:

• My only architectural suggestion is to incorporate run_scenarios() into scenarios() and have an argument to toggle this function on or off (TRUE/FALSE). It seems that the scenario class almost always needs to be populated with data by running run_scenarios() and this takes place after creating an object of the class without any changes. Therefore, incorporating the change into the constructor would save the user needing to run this extra step.
• Change is.scenario() and is.comparison() to is_scenario() and is_comparison() for consistent function naming throughout the package.
• In several places throughout the documentation you use the single quotation marks around class names, perhaps you want to use backticks here so the names are rendered as monospaced? The place this is most prevelant is comparisons.R, but I noticed it in a couple of other places as well.
• Change the # in the comparison print method to No., Num, or Number of. It was unclear to me initially that this was used for number of scenarios.
• In sce_from_json() you could use the scenario constructor instead of calling structure(). This would ensure objects of that class get initialised through the same method throughout the package. Currently, I don't think this can be done as the scenario() constructor does not accept data. But with the above-mentioned integration of run_scenarios() into this constructor it could be added.
• Check the parameter list in validate_scenario() is non-empty.
• There are no explicit tests of convenient_parameters.R functions. These functions are currently covered by testing other code. There functions are unlikely to be incorrect, or crucial to the functionality of the package, but tests may be useful anyway.
• Possible typo on test-helper_function.R:L40, should say "Testing for sce_has_data"
• Use match.args() for input checking summary_functions argument in sce_aggregrate_outcomes(). grouping_variables and measure_variables could also have input checking.
• Could add a test for expecting error for sce_from_json().

Documentation

• The variable currently named a in the example of get_outcome() could be renamed to be more informative.
• The README uses the sce_get_parameters() function, but this has been removed from the package.
• Add @description to documentation of sce_aggregate_outcomes(), this function is non-trivial and giving the user more information on what the function is doing would be helpful.
• The documentation for scenario() is focused on {finalsize}. Now you have some usage of {epidemics} in the package it could be useful to incorporate this information into the documentation of scenarios() with the possibility of an additional example using the {epidemics} package.

[pratikunterwegs]

Thanks for taking a look @joshwlambert! I've added specific fix commits in the conversations, just some general replies here. I can merge this once you sign off on it.

I've turned the package number down to 0.0.0.9000 as suggested, and noted the commit-specific {epidemics} dependency - that's a work in progress so I want to refer to a version whose outputs are known.

The missing citation should have been fixed after adding a Bib file.

I've taken away the manual addition of the logo to the website, and locally I get it looking alright with one logo now.

Re: folding run_scenario() into scenario() - I'm keeping the current implementation for now, as I'm not sure what direction this will take in the future. Offline meeting have seen this delayed evaluation feature well received, so it might continue as is.

Function names is.X() have been changed to is_X(), and the documentation now uses backticks for all scenario and comparison objects.

The # Scenarios: line in the comparison print method is now Number of ... to be clearer.

In sce_from_json(), I agree that the best way would be to use the constructor, but I'm not sure whether to allow data - this will probably be resolved as the design firms up. I haven't added an error test for now, as it's not clear we'll actually use JSON IO.

The scenario validator and constructor now have better checks for parameter lists.

sce_get_outcomes() function has a better explanation in the description, and some basic input checks - the summary functions are not limited to 'mean' and 'sd', so I am not matching those. This function too may or may not continue eventually.

The convenience functions might also go away and are mostly included for convenience during development. They could also be expanded to become more useful, of course.

I've made edits to the documentation following the suggestions, except for the focus on {finalsize} - I'm not adding anything from {epidemics} now because it's unclear what {epidemics} is supposed to achieve - this is a work in progress too.

[joshwlambert]

@pratikunterwegs Thanks for making the suggested changes. Everything looks in order to me. Will approve for this to be merged.

[pratikunterwegs]

Thanks a bunch @joshwlambert for taking the time.