Hf landscape updates [WIP] #3193
Conversation
…ded so that parameters are not resampled site-by-site in a multi-site run.
…e 35 day forecast working.
…ter ensembles are set and updated.
| } else { | ||
| PEcAn.logger::logger.error(samples.file, "not found, this file is required by the run.write.configs function") | ||
| ### use.existing.samples must be a filename | ||
| load(use.existing.samples) |
There was a problem hiding this comment.
I did a doubletake here ("wait, why is it calling load on a bool?") even after reading at the top of the function that use.existing.samples could be a filepath.
Possible alternate design: Call this param existing.sample.file, have the argument always be a path, and make the default value be NULL (behaving like FALSE does now). Then passing existing.sample.file = "samples.Rdata" would behave like TRUE does now with no need for a special case.
| # samples <- new.env() | ||
| # load(samples.file, envir = samples) ## loads ensemble.samples, trait.samples, sa.samples, runs.samples, env.samples | ||
| # trait.samples <- samples$trait.samples | ||
| # ensemble.samples <- samples$ensemble.samples | ||
| # sa.samples <- samples$sa.samples | ||
| # runs.samples <- samples$runs.samples | ||
| # ## env.samples <- samples$env.samples |
There was a problem hiding this comment.
Guessing this is temporarily commented out while developing the replacement, but I strongly recommend all replacement load calls copy the pattern shown here of loading all Rdata files into their own environment -- not only does it keep the package checks from complaining about variables with no visible binding, it means there's no ambiguity about what names were loaded from which file.
The hack of reading the file into an environment and then immediately assigning back out to separate variables was just to minimize lines changed while quieting the check note. I'd support a further refactor to use all of these directly from thee loading environment instead (e.g. write all references to sa.samples as samples$sa.samples)
There was a problem hiding this comment.
to be honest, this was me trying to resolve a merge conflict and not having time to go back and check the code in more detail (it's been months since I worked on this PR). My plan had been to take a closer look at this function and once I had pulled in the latest code
| tmp = list() | ||
| for(i in seq_len(as.numeric(settings$state.data.assimilation$chains))){ | ||
| tmp[[i]] = MCMC_data | ||
| } | ||
| MCMC_data = tmp |
There was a problem hiding this comment.
Is this equivalent to MCMC_data = rep(list(MCMC_data), settings$state.data.assimilation$chains)? That form would make it clearer to me that this is a single list of identical copies of the same list.
I haven't tested, but rep is probably more memory efficient too, because it can be allocated once and doesn't need to grow
Co-authored-by: Chris Black <chris@ckblack.org>
Description
Motivation and Context
Review Time Estimate
Types of changes
Checklist: