Add Config class with static fields

[natemcintosh]

Goal

As stated in #98, the goal is to create a Config class with static fields for all the required input parameters for the model.

You can see all of the fields for the class by creating an empty instance of it:

r$> Config()
<CFAEpiNow2Pipeline::Config>
 @ job_id            : chr(0) 
 @ task_id           : chr(0) 
 @ min_reference_date: chr(0) 
 @ max_reference_date: chr(0) 
 @ report_date       : chr(0) 
 @ as_of_date        : chr(0) 
 @ disease           : chr(0) 
 @ geo_value         : chr(0) 
 @ geo_type          : chr(0) 
 @ seed              : int(0) 
 @ horizon           : int(0) 
 @ model             : chr "EpiNow2"
 @ config_version    : chr(0) 
 @ quantile_width    : num [1:2] 0.5 0.95
 @ data              : <CFAEpiNow2Pipeline::Data>
 .. @ path                  : chr(0) 
 .. @ blob_storage_container: chr(0) 
 .. @ report_date           : chr(0) 
 .. @ reference_date        : chr(0) 
 .. @ production_date       : chr(0) 
 @ priors            :List of 2
 .. $ rt:List of 2
 ..  ..$ mean: <S7_union>: <integer> or <double>
 ..  ..$ sd  : <S7_union>: <integer> or <double>
 .. $ gp:List of 1
 ..  ..$ alpha_sd: <S7_union>: <integer> or <double>
 @ parameters        : <CFAEpiNow2Pipeline::Parameters>
 .. @ generation_interval: <CFAEpiNow2Pipeline::GenerationInterval>
 .. .. @ path                  : chr(0) 
 .. .. @ blob_storage_container: chr(0) 
 .. @ delay_interval     : <CFAEpiNow2Pipeline::DelayInterval>
 .. .. @ path                  : chr(0) 
 .. .. @ blob_storage_container: chr(0) 
 .. @ right_truncation   : <CFAEpiNow2Pipeline::RightTruncation>
 .. .. @ path                  : chr(0) 
 .. .. @ blob_storage_container: chr(0) 
 @ sampler_opts      :List of 6
 .. $ cores        : <S7_base_class>: <integer>
 .. $ chains       : <S7_base_class>: <integer>
 .. $ iter_warmup  : <S7_base_class>: <integer>
 .. $ iter_sampling: <S7_base_class>: <integer>
 .. $ max_treedepth: <S7_base_class>: <integer>
 .. $ adapt_delta  : <S7_union>: <integer> or <double>
 @ exclusions        : <CFAEpiNow2Pipeline::Exclusions>
 .. @ path                  : chr(0) 
 .. @ blob_storage_container: chr(0)

Hopefully this should make syncing changes to the fields easier.

Gotchas

To read a JSON file into the Config class, I ran into an issue created by the nested classes (e.g. Data class inside the Config class), so this uses a hard-coded solution which maps the name of the field in the Config class to the nested class, e.g. list(data = Data). If we ever add more sub-classes, they will have to be added to this hard coded list. If any knows an easier way, I'd love to use that.

Defaults

Note that S7 adds the "empty value" of the type as the default, hence the many chr(0) and int(0). In a few places, I told S7 that a field could be a union of string and NULL, to deal with reading in NULL values from the JSON. I also added defaults of actual values for

• model: "EpiNow2"
• quantile_width: c(0.5 0.95)

Feel free to change / add / remove these as necessary.

Use of lists for sampler_opts and priors

For these two, I had originally created an S7 class each, but ran into issues that required changing the pipeline code more than desired. Instead, I went back to lists, but created default values that show all of the required keys, and set the values to be the expected types. As you can see above, it should make it easy to see what is required. Because the values are classes (e.g. S7::class_integer), and not actual values like 10, it should fail loudly if the default values are used.

[github-actions]

Thank you for your contribution @natemcintosh 🚀! Your pkgdown-site is ready for download 👉 here 👈!
_{(The artifact expires on 2024-12-10T22:21:44Z. You can re-generate it by re-running the workflow here.)}

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Additional details and impacted files

📢 Thoughts on this report? Let us know!

[natemcintosh]

Looks like lintr is failing because the class names start with a capital. I had thought classes should start with a capital letter? Am willing to change this if they should be lower case.

[zsusswein]

Nah this is just a lintr default. What do the S7 docs say about naming conventions?

[zsusswein]

e.g., R6 docs follow the standard naming convention for generators and class objects but S3 uses a different style (non-encapsulated) and doesn't use that convention.

[natemcintosh]

The S7 docs seem to use UpperCamelCase in their examples. I couldn't find explicit instructions on naming anywhere though.

[zsusswein]

This is super cool. A few inline questions as I try to get my head around S7.

Can you add tests to bring patch coverage up to 100%?

[natemcintosh]

Can you add tests to bring patch coverage up to 100%?

😅 my brain conveniently forgot to write tests. Will add tests.

[natemcintosh]

Ok, things are looking pretty good. I am left confused by the warning from R CMD check however. There's some formatting difference between docstrings and the .Rd files? Searching for this online leads me down rabbit holes, and no direct answers.

[natemcintosh]

After reviewing the differences in a differ, they are indeed all formatting differences.

Ahhhhhhh

[natemcintosh]

Ok, it looks like running pre-commit run -a has to remove white space at the end of lines, and running devtools::check() changes Config.Rd back to what it used to be.

[zsusswein]

I think you need to run Rscript -e "roxygen2::roxygenize()" to re-render the docs (that should fix the R CMD check problems) and then add https://github.com/CDCgov/cfa-gam-rt/blob/2f5f94a57e16b3c5e51213d01da562bddc2f05ec/.pre-commit-config.yaml#L29 to the hooks to prevent it from messing with the formatting.

[natemcintosh]

It looks like that line is already in the repo. Running roxygen2::roxygenize(); devtools::check() doesn't seem to fix the issue sadly

[zsusswein]

Ok noting that I can reproduce the R CMD check warning. Looking into it a bit.

[natemcintosh]

Looks like it just be that Roxygen2 does not yet support S7. Thanks @damonbayer for noticing this.

If this is the issue, then I think our choices are:

1. Stick with S7, and merge over Roxygen's protests
2. Re-write in e.g. S3, and don't have to deal with this
3. Remove the default values for the list fields. This will make it harder to know what fields are required in them, but our documentation can supplement that until S7 and Roxygen get along better.

At this point, I think I would go for 3.

[damonbayer]

@natemcintosh you could manually modify the usage section of the documentation files.

[natemcintosh]

@natemcintosh you could manually modify the usage section of the documentation.

As in manually change whitespace differences?

[damonbayer]

@natemcintosh you could manually modify the usage section of the documentation.

As in manually change whitespace differences?

I thought the problem was all of this bonus stuff in the usage section of the Rd file:

cfa-epinow2-pipeline/man/Config.Rd

Lines 25 to 160 in 0c12e82

	.data, validator = function (object)
	{
	if (base_class(object) != name) {
	sprintf("Underlying data must be <\%s> not <\%s>", name, base_class(object))
	}
	}), class = "S7_base_class"), structure(list(class = "double", constructor_name =
	"double", constructor = function (.data = numeric(0))
	.data, validator = function
	(object)
	{
	if (base_class(object) != name) {
	sprintf("Underlying data must be <\%s> not <\%s>", name, base_class(object))
	}
	}), class = "S7_base_class"))), class = "S7_union"), sd = structure(list(classes =
	list(structure(list(class = "integer", constructor_name = "integer", constructor =
	function (.data = integer(0))
	.data, validator = function (object)
	{
	if
	(base_class(object) != name) {
	sprintf("Underlying data must be <\%s> not <\%s>", name, base_class(object))
	}
	}), class = "S7_base_class"), structure(list(class = "double", constructor_name =
	"double", constructor = function (.data = numeric(0))
	.data, validator = function
	(object)
	{
	if (base_class(object) != name) {
	sprintf("Underlying data must be <\%s> not <\%s>", name, base_class(object))
	}
	}), class = "S7_base_class"))), class = "S7_union")), gp = list(alpha_sd =
	structure(list(classes = list(structure(list(class = "integer", constructor_name =
	"integer", constructor = function (.data = integer(0))
	.data, validator = function
	(object)
	{
	if (base_class(object) != name) {
	sprintf("Underlying data must be <\%s> not <\%s>", name, base_class(object))
	}
	}), class = "S7_base_class"), structure(list(class = "double", constructor_name =
	"double", constructor = function (.data = numeric(0))
	.data, validator = function
	(object)
	{
	if (base_class(object) != name) {
	sprintf("Underlying data must be <\%s> not <\%s>", name, base_class(object))
	}
	}), class = "S7_base_class"))), class = "S7_union"))),
	parameters = Parameters(),
	sampler_opts = list(cores = structure(list(class = "integer", constructor_name =
	"integer", constructor = function (.data = integer(0))
	.data, validator = function
	(object)
	{
	if (base_class(object) != name) {
	sprintf("Underlying data must be <\%s> not <\%s>", name, base_class(object))
	}
	}), class = "S7_base_class"), chains = structure(list(class = "integer",
	constructor_name = "integer", constructor = function (.data = integer(0))
	.data,
	validator = function (object)
	{
	if (base_class(object) != name) {
	sprintf("Underlying data must be <\%s> not <\%s>", name, base_class(object))
	}
	}), class = "S7_base_class"), iter_warmup = structure(list(class = "integer",
	constructor_name = "integer", constructor = function (.data = integer(0))
	.data,
	validator = function (object)
	{
	if (base_class(object) != name) {
	sprintf("Underlying data must be <\%s> not <\%s>", name, base_class(object))
	}
	}), class = "S7_base_class"), iter_sampling = structure(list(class = "integer",
	constructor_name = "integer", constructor = function (.data = integer(0))
	.data,
	validator = function (object)
	{
	if (base_class(object) != name) {
	sprintf("Underlying data must be <\%s> not <\%s>", name, base_class(object))
	}
	}), class = "S7_base_class"), max_treedepth = structure(list(class = "integer",
	constructor_name = "integer", constructor = function (.data = integer(0))
	.data,
	validator = function (object)
	{
	if (base_class(object) != name) {
	sprintf("Underlying data must be <\%s> not <\%s>", name, base_class(object))
	}
	}), class = "S7_base_class"), adapt_delta = structure(list(classes =
	list(structure(list(class = "integer", constructor_name = "integer", constructor =
	function (.data = integer(0))
	.data, validator = function (object)
	{
	if
	(base_class(object) != name) {
	sprintf("Underlying data must be <\%s> not <\%s>", name, base_class(object))
	}
	}), class = "S7_base_class"), structure(list(class = "double", constructor_name =
	"double", constructor = function (.data = numeric(0))
	.data, validator = function
	(object)
	{
	if (base_class(object) != name) {
	sprintf("Underlying data must be <\%s> not <\%s>", name, base_class(object))
	}
	}), class = "S7_base_class"))), class = "S7_union")),
	exclusions = Exclusions()
	)
	}

My suggestion was to remove that by hand (or automate its removal). I could be misunderstanding the issue, though.

[natemcintosh]

After a little more playing around, I discovered that removing the default values for two lists inside Config resolved the issue. I'll add more detail on the issue for adding S7 support in Roxygen.

But I will also update the list of possible actions we can take above.

[kgostic]

I asked one question but otherwise LGTM. Please have Zack look too before merge!

😎