Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code formatting + style cleanup in sipnet, workflow, ERA5 #3407

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
+1,425 −1,202
Open

Code formatting + style cleanup in sipnet, workflow, ERA5 #3407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
582f9b5
move duplicate code out of both legs of if/else, line wrapping
infotroph Jan 4, 2025
578c20c
ran styler on ERA5 fns
infotroph Jan 4, 2025
04139f8
run styler on all files in models/sipnet
infotroph Jan 4, 2025
6b95332
run styler on workflow pkg
infotroph Jan 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 58 additions & 48 deletions base/workflow/R/create_execute_test_xml.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,14 @@ create_execute_test_xml <- function(model_id,
db_bety_hostname = NULL,
db_bety_port = NULL,
db_bety_driver = "Postgres") {

php_file <- file.path(pecan_path, "web", "config.php")
config.list <- PEcAn.utils::read_web_config(php_file)
if (is.null(db_bety_username)) db_bety_username <- config.list$db_bety_username
if (is.null(db_bety_password)) db_bety_password <- config.list$db_bety_password
if (is.null(db_bety_hostname)) db_bety_hostname <- config.list$db_bety_hostname
if (is.null(db_bety_port)) db_bety_port <- config.list$db_bety_port
#opening a connection to bety

# opening a connection to bety
con <- PEcAn.DB::db.open(list(
user = db_bety_username,
password = db_bety_password,
Expand All @@ -65,17 +64,19 @@ create_execute_test_xml <- function(model_id,
on.exit(PEcAn.DB::db.close(con), add = TRUE)

settings <- list(
info = list(notes = "Test_Run",
userid = user_id,
username = "None",
dates = Sys.Date())
info = list(
notes = "Test_Run",
userid = user_id,
username = "None",
dates = Sys.Date()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the updated version is harder to read because the elements in the list don't line up with the parenthesis

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're in good company -- I see hanging indents for function calls in many codebases and {lintr} accepts either style, however the tidyverse style guide is explicit about recommending against them for calls even though it accepts them for definitions. With the disclaimer that I conveniently like the single-indent style better anyway, I do see virtue in just following the style guide here.

Also a PEcAn specific point: A lot of our function calls have very long names and assign their results into deeply nested objects, so a hanging indent on something like some$list$sublist$item <- PEcAn.data.atmosphere::extract_thing_from_stuff(
is 75 characters (plus any nesting for the block it appears in) of whitespace before we even start writing arguments, so at my usual window widths many well-intentioned hanging indents tend to get mangled by linewrapping.

BTW, I think we agree it is important that all arguments get the same indentation. We have a lot of places we used to have nice alignments until we added namespaces to the call without adjusting the wrapped lines beneath them:

- somefun(a = foo,
+ somepkg::somefun(a = foo,
          b = bar,
          c = FALSE)

I'm happy to stipulate that these need to be fixed whatever style we're using.

)
)

#Outdir
# Outdir
model.new <- dplyr::tbl(con, "models") %>%
dplyr::filter(.data$id == !!model_id) %>%
dplyr::collect()

outdir_pre <- paste(
model.new[["model_name"]],
format(as.Date(start_date), "%Y-%m"),
Expand All @@ -90,75 +91,84 @@ create_execute_test_xml <- function(model_id,
outdir <- normalizePath(outdir)
settings$outdir <- outdir

#Database BETY
# Database BETY
settings$database <- list(
bety = list(user = db_bety_username,
password = db_bety_password,
host = db_bety_hostname,
dbname = "bety",
driver = db_bety_driver,
write = FALSE),
bety = list(
user = db_bety_username,
password = db_bety_password,
host = db_bety_hostname,
dbname = "bety",
driver = db_bety_driver,
write = FALSE
),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same. I do like the closing parenthesis on it's own line, but not indented at the same level as the original variable

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I had my druthers I'd use single-indent but with the closing paren always on the same line as the last argument (like in hanging indent), but I seem to be alone one that one.

dbfiles = dbfiles_folder
)

#PFT
if (is.null(pft)){
# PFT
if (is.null(pft)) {
# Select the first PFT in the model list.
pft <- dplyr::tbl(con, "pfts") %>%
dplyr::filter(.data$modeltype_id == !!model.new$modeltype_id) %>%
dplyr::collect()

pft <- pft$name[[1]]
message("PFT is `NULL`. Defaulting to the following PFT: ",
pft)
message(
"PFT is `NULL`. Defaulting to the following PFT: ",
pft
)
}

## Putting multiple PFTs separated by semicolon
settings$pfts <- strsplit(pft, ";")[[1]] %>%
purrr::map( ~ list(name = .x,
constants = list(num = 1)
)
) %>%
stats::setNames(rep("pft", length(.data)))
purrr::map(~ list(
name = .x,
constants = list(num = 1)
)) %>%
stats::setNames(rep("pft", length(.data)))

#Meta Analysis
# Meta Analysis
settings$meta.analysis <- list(iter = 3000, random.effects = FALSE)

#Ensemble
# Ensemble
settings$ensemble <- list(
size = ensemble_size,
variable = sensitivity_variable,
samplingspace = list(met = list(method = "sampling"),
parameters = list(method = "uniform"))
samplingspace = list(
met = list(method = "sampling"),
parameters = list(method = "uniform")
)
)

#Sensitivity
# Sensitivity
if (sensitivity) {
settings$sensitivity.analysis <- list(
quantiles = list(sigma1 = -2, sigma2 = -1, sigma3 = 1, sigma4 = 2)
)
}

#Model
# Model
settings$model$id <- model.new[["id"]]

#Workflow
# Workflow
settings$workflow$id
settings$workflow$id <- paste0("Test_run_","_",model.new$model_name)
settings$workflow$id <- paste0("Test_run_", "_", model.new$model_name)
settings$run <- list(
site = list(id = site_id, met.start = start_date, met.end = end_date),
inputs = list(met = list(source = met, output = model.new[["model_name"]],
username = "pecan")),
inputs = list(met = list(
source = met, output = model.new[["model_name"]],
username = "pecan"
)),
start.date = start_date, end.date = end_date
)
settings$host$name <- "localhost"


# Add model specific options
settings<-model_specific_tags(settings, model.new)
#create file and Run
settings <- model_specific_tags(settings, model.new)
# create file and Run
XML::saveXML(PEcAn.settings::listToXml(settings, "pecan"),
file = file.path(outdir, "pecan.xml"))
file = file.path(outdir, "pecan.xml")
)
file.copy(file.path(pecan_path, "web", "workflow.R"), outdir)
cwd <- getwd()
setwd(outdir)
Expand All @@ -180,14 +190,14 @@ create_execute_test_xml <- function(model_id,
#' @return updated settings list
#' @export
#'
model_specific_tags <- function(settings, model.info){

#some extra settings for LPJ-GUESS
if(model.info$model_name=="LPJ-GUESS"){
settings$run$inputs <- c(settings$run$inputs ,
list(soil=list(id=1000000903))
)
model_specific_tags <- function(settings, model.info) {
# some extra settings for LPJ-GUESS
if (model.info$model_name == "LPJ-GUESS") {
settings$run$inputs <- c(
settings$run$inputs,
list(soil = list(id = 1000000903))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what the aim or this overall function is, or where it's called from, but this sort of deeply-buried hard-coded hack is something we should really avoid. I know you didn't add this and are just reformatting it, but I wonder if we could take the opportunity to fix it? First, this bit won't work if we're moving away from BETY being required (and never would have worked on a machine lacking this input ID). Second, as we move towards more people passing in ensemble soil inputs, this is going to break things (especially if a soil input is already present, which the code never checks for)

Copy link
Member Author

@infotroph infotroph Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 to seeing something and taking the chance to fix it, but let's do it in a separate PR -- "Here's a big pile of whitespace changes with a few behavior changes mixed in" is my least favorite diff to review. I've made an issue for this in #3410

)
}

return(settings)
}
76 changes: 42 additions & 34 deletions base/workflow/R/do_conversions.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,61 +12,61 @@ do_conversions <- function(settings, overwrite.met = FALSE, overwrite.fia = FALS
if (PEcAn.settings::is.MultiSettings(settings)) {
return(PEcAn.settings::papply(settings, do_conversions))
}

needsave <- FALSE
if (is.character(settings$run$inputs)) {
settings$run$inputs <- NULL ## check for empty set
settings$run$inputs <- NULL ## check for empty set
}

dbfiles.local <- settings$database$dbfiles
dbfiles <- ifelse(!PEcAn.remote::is.localhost(settings$host) & !is.null(settings$host$folder), settings$host$folder, dbfiles.local)
PEcAn.logger::logger.debug("do.conversion outdir",dbfiles)
PEcAn.logger::logger.debug("do.conversion outdir", dbfiles)
# For each input
for (i in seq_along(settings$run$inputs)) {
input <- settings$run$inputs[[i]]
if (is.null(input)) {
next
}

input.tag <- names(settings$run$input)[i]
PEcAn.logger::logger.info("PROCESSING: ",input.tag)
PEcAn.logger::logger.info("PROCESSING: ", input.tag)


ic.flag <- fia.flag <- FALSE
if ((input.tag %in% c("css", "pss", "site")) &&
is.null(input$path) && !is.null(input$source)) {
if(!is.null(input$useic)){ # set <useic>TRUE</useic> if IC Workflow, leave empty if not
ic.flag <- TRUE
}else if(input$source == "FIA"){

if ((input.tag %in% c("css", "pss", "site")) &&
is.null(input$path) && !is.null(input$source)) {
if (!is.null(input$useic)) { # set <useic>TRUE</useic> if IC Workflow, leave empty if not
ic.flag <- TRUE
} else if (input$source == "FIA") {
fia.flag <- TRUE
# possibly a warning for deprecation in the future
}
}

# BADM IC
if(input.tag == "poolinitcond" && is.null(input$path)){
ic.flag <- TRUE
if (input.tag == "poolinitcond" && is.null(input$path)) {
ic.flag <- TRUE
}

# IC conversion : for now for ED only, hence the css/pss/site check
# <useic>TRUE</useic>
if (ic.flag) {
settings <- PEcAn.data.land::ic_process(settings, input, dir = dbfiles, overwrite = overwrite.ic)
settings <- PEcAn.data.land::ic_process(settings, input, dir = dbfiles, overwrite = overwrite.ic)
needsave <- TRUE
}

# keep fia.to.psscss
if (fia.flag) {
settings <- PEcAn.data.land::fia.to.psscss(settings, overwrite = overwrite.fia)
needsave <- TRUE
}





# soil extraction
if(input.tag == "soil" && is.null(input$path)){
settings$run$inputs[[i]]$path <- PEcAn.data.land::soil_process(settings, input, dbfiles.local, overwrite=FALSE)
if (input.tag == "soil" && is.null(input$path)) {
settings$run$inputs[[i]]$path <- PEcAn.data.land::soil_process(settings, input, dbfiles.local, overwrite = FALSE)
needsave <- TRUE
## NOTES: at the moment only processing soil locally. Need to think about how to generalize this
## because many models will read PEcAn standard in write.configs and write out into settings
Expand All @@ -75,30 +75,38 @@ do_conversions <- function(settings, overwrite.met = FALSE, overwrite.fia = FALS
}

# Phenology data extraction
if(input.tag == "leaf_phenology" && is.null(input$path)){
#settings$run$inputs[[i]]$path <- PEcAn.data.remote::extract_phenology_MODIS(site_info,start_date,end_date,outdir,run_parallel = TRUE,ncores = NULL)
if (input.tag == "leaf_phenology" && is.null(input$path)) {
# settings$run$inputs[[i]]$path <- PEcAn.data.remote::extract_phenology_MODIS(
infotroph marked this conversation as resolved.
Show resolved Hide resolved
# site_info,
# start_date,
# end_date,
# outdir,
# run_parallel = TRUE,
# ncores = NULL
# )
needsave <- TRUE
}

# met conversion

if (input.tag == "met") {
name <- "MET Process"
if ( (PEcAn.utils::status.check(name) == 0)) { ## previously is.null(input$path) &&
PEcAn.logger::logger.info("calling met.process: ",settings$run$inputs[[i]][['path']])
settings$run$inputs[[i]] <-
if ((PEcAn.utils::status.check(name) == 0)) { ## previously is.null(input$path) &&
PEcAn.logger::logger.info("calling met.process: ", settings$run$inputs[[i]][["path"]])
settings$run$inputs[[i]] <-
PEcAn.data.atmosphere::met.process(
site = settings$run$site,
site = settings$run$site,
input_met = settings$run$inputs$met,
start_date = settings$run$start.date,
end_date = settings$run$end.date,
model = settings$model$type,
host = settings$host,
dbparms = settings$database$bety,
dbparms = settings$database$bety,
dir = dbfiles,
spin = settings$spin,
overwrite = overwrite.met)
PEcAn.logger::logger.debug("updated met path: ",settings$run$inputs[[i]][['path']])
overwrite = overwrite.met
)
PEcAn.logger::logger.debug("updated met path: ", settings$run$inputs[[i]][["path"]])
needsave <- TRUE
}
}
Expand Down
Loading
Loading