LPJ-GUESS restart support: new write_restart, read_restart tweaks & CRU driver hooks #3533

yinghaoSunn · 2025-05-30T06:57:06Z

Description

This PR completes the restart workflow for the LPJ-GUESS model in PEcAn.

Motivation and Context

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)

…g pipeline * implement write_restart.LPJGUESS() with binary-state update * adjust read_restart.LPJGUESS() * extend write.config.LPJGUESS() – CRU bin / misc placeholders (@CLIMATEFILE@, @miscfile@) * split_inputs.LPJGUESS(): bypass *.cru / *.cru.bin files * add template.ins placeholder @miscfile@; update pecan.ins * docs + NAMESPACE entries

mdietze

Build is failing on dplyr, so that's the key requested change

mdietze · 2025-05-30T10:32:23Z

models/lpjguess/R/write_restart.LPJGUESS.R

+##' 
+##' @return NONE
+##'
+##' @importFrom dplyr %>%


At this point, I'd recommend shifting to base R pipes, |>, rather than dplyr pipes, %>%, which will eliminate the need to add an additional package dependency

I have deleted the dplyr cause we don't need it.

mdietze · 2025-05-30T10:34:35Z

models/lpjguess/R/read_restart.LPJGUESS.R

+#' @export
+#' @examples
+#' # example code
+#' outdir = "/fs/data2/output//PEcAn_1000010473/out"


I'd recommend putting the example in a "donotrun" block so that the package build doesn't attempt to run this code, which will fail on the Github cloud which doesn't have access to any of these paths and files

Also, the example code doesn't actually show how to call the function, which is what should go in an example

mdietze · 2025-05-30T10:38:12Z

models/lpjguess/R/read_state.R

 #' @param if_else_check Optional. A logical value indicating whether to check for if/else blocks (default is FALSE).
 #' @export
 #' @return A numeric value indicating the line number of the matching closing bracket.
+#' @keywords internal


Why does this function have the keyword "internal" but also "@export", which makes it an exported (i.e. external) function

Deleted the @keywords internal.

mdietze · 2025-05-30T10:40:29Z

models/lpjguess/R/read_state.R

+#' @param ... Parts of a nested list path (e.g., "Gridcell", "Stand", 1, "Patch", 2)
+#' @return A single string key like "Gridcell/Stand/1/Patch/2"
+#' @keywords internal
+make_key <- function(...) paste(..., sep = "/")


How is this function different from file.path?

Good point. I forgot file.path can also set fsep as "/"

mdietze · 2025-05-30T10:48:08Z

models/lpjguess/R/write.config.LPJGUESS.R

+  # when using cru input, lpjguess will not use these clim files
+  cru.file <- settings$run$inputs$met$path
+  misc.file <- sub("\\.bin$", "misc.bin", cru.file)
+  guessins <- gsub("@SOIL_FILE@", cru.file, guessins)


Why is the met data path being assigned to tag named @SOIL_FILE@? This is very unintuitive and suggests that the guessins template needs updating

This is a legacy from when we used NetCDF input, where met and soil data were separate and @SOIL_FILE@ pointed to the soil input.

Now that we’ve switched to CRU input, the .cru.bin file actually provides both met and soil data, so we reused @SOIL_FILE@ for simplicity.

You're right though — renaming the placeholder to something like @MET_AND_SOIL_FILE@ or @CRU_FILE@ would be more intuitive and better documented. I can update the template accordingly.

mdietze · 2025-05-30T10:52:05Z

models/lpjguess/R/write.config.LPJGUESS.R

-  guessins <- gsub("@SOIL_FILE@", soil.file, guessins)
+  # # write soil file path
+  # # when using cru input, it's also climate file
+  # soil.file <- settings$run$inputs$soil$path


Similarly, curious why the code setting the soils data is commented out. Is this just temporary? Presumably we'll want PEcAn to have the ability to pass in site-specific soil information.

Yes, it's temporary. We’ve switched to CRU input, and the .cru.bin file actually provides both met and soil data.

mdietze · 2025-05-30T10:56:56Z

models/lpjguess/R/write_restart.LPJGUESS.R

+                file.path(outdir, runid, paste0("lpjguess.", as.Date(start.time), ".out")))
+    system(paste("rm", file.path(rundir, runid, "lpjguess.clim")))
+  } else {
+    print(paste("Files not renamed -- Need to rerun timestep", start.time, "before next time step"))


this is a surprising flag to post in response to a boolean input flag that is set by the user

it's also surprising that a flag that tells you to rerun a timestep doesn't stop the code

error messages should use the PEcAn.logger package

Thanks — you're right that the logic here was misleading. I've replaced the print() with logger.severe() and now explicitly stop() if RENAME == FALSE.

mdietze · 2025-05-30T11:18:12Z

models/lpjguess/R/write_restart.LPJGUESS.R

+
+
+
+  ## ---- Build PFT parameter table from new.params ----


Are these parameters that are needed to be able to write the restart file (e.g. convert new.state to model specific outputs) or just to parameters needed to write the param file? Presumably the latter has not changed from what was written during write.configs. For the former, the custom has been to pass these through new.params. It makes me nervous to see a whole bunch of parameters hard-coded in write_restart

Short-term method for testing.

mdietze · 2025-05-30T11:21:55Z

models/lpjguess/inst/pecan.ins

 run_peatland 1	      ! whether to simulate peatland (1) or not (0)

-@@@@@ Remove in PalEON version @@@@@
+! Remove in PalEON version 


not that write.config is looking for the tag "##### Remove in PalEON version #####" not "! Remove in PalEON version" so I don't think these will be removed

github-actions bot added the Models label May 30, 2025

mdietze requested changes May 30, 2025

View reviewed changes

yinghaoSunn added 2 commits May 30, 2025 21:25

Delete dplyr %>% in write_restart.LPJGUESS

97c11f7

Fix some issues.

5b99c00

github-actions bot added the Modules label May 30, 2025

yinghaoSunn force-pushed the hoptest branch from be01514 to 5b99c00 Compare May 30, 2025 21:43

mdietze approved these changes May 31, 2025

View reviewed changes

mdietze added this pull request to the merge queue May 31, 2025

Merged via the queue into PecanProject:develop with commit 7faa280 May 31, 2025
19 of 25 checks passed

LPJ-GUESS restart support: new write_restart, read_restart tweaks & CRU driver hooks #3533

LPJ-GUESS restart support: new write_restart, read_restart tweaks & CRU driver hooks #3533

Uh oh!

Conversation

yinghaoSunn commented May 30, 2025

Description

Motivation and Context

Review Time Estimate

Types of changes

Uh oh!

mdietze left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants