Skip to content

Integrate LPJ-GUESS with SDA module and synchronize with latest upstream#3914

Merged
mdietze merged 7 commits intoPecanProject:developfrom
yinghaoSunn:develop
Apr 6, 2026
Merged

Integrate LPJ-GUESS with SDA module and synchronize with latest upstream#3914
mdietze merged 7 commits intoPecanProject:developfrom
yinghaoSunn:develop

Conversation

@yinghaoSunn
Copy link
Copy Markdown
Contributor

@yinghaoSunn yinghaoSunn commented Apr 2, 2026

Description

This PR implements the coupling of LPJ-GUESS within the PEcAn Sequential Data Assimilation (SDA) workflow. It includes critical updates to state handling, binary data extraction, and ensures full compatibility with the latest PEcAn develop branch:

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)

Checklist:

  • My change requires a change to the documentation.
  • My name is in the list of CITATION.cff
  • I agree that PEcAn Project may distribute my contribution under any or all of
    • the same license as the existing code,
    • and/or the BSD 3-clause license.
  • 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.

Merge branch 'my-work-backup-20260331' into develop

# Conflicts:
#	modules/assim.sequential/R/Adjustment.R
#	modules/assim.sequential/R/Analysis_sda_block.R
#	modules/assim.sequential/R/sda.enkf_MultiSite.R
#	modules/uncertainty/R/ensemble.R
#	modules/uncertainty/R/get.parameter.samples.R
@github-actions github-actions bot added the models label Apr 2, 2026
Copy link
Copy Markdown

@Yeligay8 Yeligay8 left a comment

Choose a reason for hiding this comment

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

In CI / check (check_models, 4.2) / check (pull_request) error:
Status: BROKEN
Error: Please fix these and resubmit.

Main blocking issue:
checking R files for non-ASCII characters ... WARNING

File: update.state.both.LPJGUESS.R

I suggest to fix to:
grep -n "[^ -~]" models/lpjguess/R/update.state.both.LPJGUESS.R

OR

tools::showNonASCIIfile("models/lpjguess/R/update.state.both.LPJGUESS.R")

Also, maybe replacing characters might help:
“ ” to ", ‘ ’ to ' , — to -, µ to u

Here I guess R doesn't know where str () comes from:
no visible global function definition for ‘str’

I suggest: importFrom(utils, str)

I guess here needs fixing in missing documentation:
checking for missing documentation entries ... WARNING

run:
devtools::document()
then check for missing docs:
devtools::check()

Here needs fixing in empty Rd section:
Dropping empty section \details

you can edit to:
dot-seed_cohort_quick.Rd

Remove empty section:
\details{
}

Fix ASCII:
tools::showNonASCIIfile("models/lpjguess/R/update.state.both.LPJGUESS.R")

Fix import and add:
#' @importFrom utils str

Regenerate docs:
devtools::document()

Re-run checks locally:
devtools::check()

Run Make again:
make -j1 check_models

@yinghaoSunn yinghaoSunn requested a review from Yeligay8 April 6, 2026 21:23
@yinghaoSunn
Copy link
Copy Markdown
Contributor Author

@Yeligay8 Thanks for the detailed review. I’ve addressed the issues you pointed out, including the non-ASCII characters and missing documentation, and the CI / check (check_models, 4.2) check is now passing. The remaining failing checks appear to be non-required Docker builds. Could you please take another look when you have a chance?

@mdietze mdietze dismissed Yeligay8’s stale review April 6, 2026 21:29

Not a dev team member. Issues have been addressed. Likely that the review was LLM generated.

@mdietze mdietze enabled auto-merge April 6, 2026 21:30
@mdietze mdietze added this pull request to the merge queue Apr 6, 2026
Merged via the queue into PecanProject:develop with commit 20bd132 Apr 6, 2026
18 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants