initial revisions to write_params_ctsm function

[dlebauer]

Ready to merge

references #2301

Review Time Estimate

• Immediately
• Within one week
• When possible

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My change requires a change to the documentation.
• I have updated the CHANGELOG.md.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[KristinaRiemer]

@dlebauer why does this function name use underscores instead of periods?

[ashiklom]

why does this function name use underscores instead of periods?

@KristinaRiemer Apologies if someone already answered this offline, and additional apologies for the long-winded answer, but...

R uses dots in function names as part of its S3 method system, which allows functions of the same name to do different things for different objects. For example, seq will do different things when given a numeric input or a date input...

seq(1, 5)
#> [1] 1 2 3 4 5
seq(as.Date("2010-01-01"), as.Date("2010-02-01"), by = "3 days")
#>  [1] "2010-01-01" "2010-01-04" "2010-01-07" "2010-01-10" "2010-01-13"
#>  [6] "2010-01-16" "2010-01-19" "2010-01-22" "2010-01-25" "2010-01-28"
#> [11] "2010-01-31"

...because under the hood, it calls seq.default in the first case and seq.Date in the second case.

In general, the S3 method system uses the naming convention function.class -- i.e. seq.Date is a method of the generic function seq for the class Date. Similarly, print.data.frame is the print method for an object of class data.frame.

Unfortunately, in what I think is arguably the single worst design decision in R, this only happens when methods have been registered; otherwise, R will let you put as many dots in object names as you want. This can lead to ambiguity; for instance, is data.frame its own function, or the data method for an object of class frame? Similarly, is t.test its own function, or the t method for an object of class test? More scarily, it's very easy to accidentally overwrite these. Note the errors that happen below:

data <- function(...) UseMethod("data")
x <- structure(list(x = 1, y = 2), class = "frame")
class(x)
#> [1] "frame"
data(x)
#> Error in as.data.frame.default(x[[i]], optional = TRUE, stringsAsFactors = stringsAsFactors): cannot coerce class '"frame"' to a data.frame

t <- function(...) UseMethod("t")
y <- structure(5, class = "test")
t(y)
#> Error in t.test.default(y): not enough 'x' observations

In both cases, we end up unintentionally calling data.frame and t.test functions.

So, in general, that's why we avoid dots in function names.

[KristinaRiemer]

@ashiklom that's a great explanation, thanks for taking the time! I think I knew vaguely that using dots when naming functions was tricky, but I didn't know why.

As a followup that, some of the Pecan functions have dots in them. Are they using the S3 system? All of the models in Pecan have write.config functions. So for write.config.ED2, is ED2 the class and write.config the function?

[ashiklom]

As a followup that, some of the Pecan functions have dots in them. Are they using the S3 system?

No, those aren't S3 methods, but rather a legacy of sloppy development. We've kept them around as long as we have for backwards compatibility -- i.e. to avoid breaking all the external custom scripts people have written that use those functions. In fact, replacing . with _ in function names is on our list of backwards-incompatible changes for PEcAn 2.0 (#392).

In the meantime, trying to change these names may get you yelled at by your supervisor 😜

[KristinaRiemer]

Gotcha! Okay, that really addresses my confusion about the inconsistency with using dots vs underscores. And now I know more about how/when to use dots. Thank so much!

[KristinaRiemer]

Looking for feedback on this function now. A couple of things to pay attention to:

1. The conversion on line 92 could probably be done in a better way
2. Similarly, the refactoring of the variable conversion (lines 85-92)
3. Need to make sure nothing crucial was deleted

[ashiklom]

Looks OK to me, but I haven't actually tested it. Might not be a bad idea for someone with more knowledge of CTSM to take a look.

Eventually, it would be good to add a unit test for this, but that shouldn't hold up this PR.

[serbinsh]

I would instead advocate that we use a refrun script, like the one I wrote for the Docker, that places the clm param file in the refrun directory such that we have the correct version in the refrun directory. Then we grab the default file from that directory instead of the pecan package directory

[serbinsh]

example: https://github.com/serbinsh/ctsm_containers/blob/master/ctsm_run_scripts/create_case_ctsmfates_1pt_example_PA-SLZ.sh

where we would have something like:

export CLM_PARAM_FILE_PATH={path to cesm datasets/lnd/clm2/}
export CLM_PARAM_FILE=clm5_params.c171117_0001.nc /new/run/dir

in the create refrun script for the Docker image and then the param file will be in that dir on the host machine

[KristinaRiemer]

@serbinsh I don't think I quite understand this. I've looked over your example, but that is part of creating a case for a CLM run, which is not what we want to do here?

So we want to have a shell script with the two export lines (in models/ctsm?). This will indicate where the initial CLM parameter file is, which will not be in the Pecan repository but will only exists as a host machine file? Will it be an "inputs"-type file like in Bety? And will that shell script then be run by a Dockerfile? Which one?

[dlebauer]

@KristinaRiemer please move shawn's suggestions to new PR and then this can be merged

[serbinsh]

so we would update this as well

[serbinsh]

See comments in line

[KristinaRiemer]

Moving refrun directory conversation over to a new issue #2388.

[mdietze]

@serbinsh have your requested changes been addressed?

[serbinsh]

Moving refrun directory conversation over to a new issue #2388.

Great, thanks @KristinaRiemer

[serbinsh]

I think this looks OK but need to actually try and run it with a ctsm model soon to confirm its giving us what we need! We will also need to replace the defaults netCDF with one that comes with CTSM

[serbinsh]

why is this here? copy and past issue?