Skip to content

Conversation

DongchenZ
Copy link
Contributor

Description

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.

@DongchenZ DongchenZ requested review from infotroph and mdietze and removed request for mdietze October 3, 2025 13:47
@DongchenZ DongchenZ requested a review from mdietze October 3, 2025 13:47
@mdietze mdietze enabled auto-merge October 3, 2025 14:00
@mdietze mdietze added this pull request to the merge queue Oct 3, 2025
Copy link
Member

@mdietze mdietze left a comment

Choose a reason for hiding this comment

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

FYI, this provides an immediate fix, but really you should be passing in the new input_design object too, not just the new ensemble.size variable. That change is needed to get the desired result of sampling jointly once at the start of the SDA and then keeping that design as the SDA iterates.

@mdietze mdietze removed this pull request from the merge queue due to a manual request Oct 3, 2025
Copy link
Member

@mdietze mdietze left a comment

Choose a reason for hiding this comment

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

Just checked are was reminded that input_design is now the first argument to write.ensemble.configs and it's not optional, so adding that fix is actually required in this PR, not just a good next step for a future PR.

write.ensemble.configs <- function(input_design , ensemble.size, defaults, ensemble.samples, settings, model,

I'd recommend that you add a call to modules/uncertainty/R/generate_joint_ensemble_design once in your code early on, that you save the design to restart, and that you read that design during restart rather than generating a new one (i.e., that if you have an "if restart" option, then the input_design is generated in the
"else")

@DongchenZ DongchenZ changed the title Bug fixes for the missing ensemble.size error. Implement the join_inputs_design into the current SDA workflows. Oct 6, 2025
TimeseriesPlot = FALSE,
debug = FALSE,
pause = FALSE,
Profiling = FALSE,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed unnecessary flags.

control=list(TimeseriesPlot = FALSE,
OutlierDetection=FALSE,
parallel_qsub = TRUE,
parallel_qsub = FALSE,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set the parallel_qsub as FALSE because we can't use both local and remote qsub for the model execution.

run_parallel = TRUE,
MCMC.args = NULL),
MCMC.args = NULL,
local.execution = TRUE),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add the local.execution when we just want to run the jobs locally.

@github-actions github-actions bot added the Base label Oct 19, 2025
"latin-ascii"
)
if (nchar(long.msg) > 20 && wrap) {
if (wrap) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The hard-coded length in the if control should be removed. The wrap argument should fully define it.


export(met2model.STICS)
export(model2netcdf.STICS)
export(pecan2stics)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is generated by building the existing pecan files.

Copy link
Contributor Author

@DongchenZ DongchenZ left a comment

Choose a reason for hiding this comment

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

The newest commits provide the new debias workflow (which has already been implemented in the current SDA workflows), and the old script has been moved to the inst folder. This is cleaner and easier to understand, which could also benefit any future development of the debiasing algorithm.

}
cores <- cores - 1
# if we only have one CPU.
if (cores < 1) cores <- 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update the parallel registration.

#' `qsub_parallel`, where we first combine jobs and submit them into the SCC.
#' @param debias List: R list containing the covariance directory and the start year.
#' covariance directory should include GeoTIFF files named by year.
#' start year is numeric input which decide when to start the debiasing feature.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the previous debias workflow and replaced it with the new workflow.

@github-actions github-actions bot added the Tests label Oct 21, 2025
}
PEcAn.logger::logger.info("SMAP SMP Prep Completed!")
list(SMP_Output = SMAP_Output, time_points = time_points, var = "SoilMoist")
list(SMP_Output = SMAP_Output, time_points = time_points, var = "SoilMoistFrac")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct the variable name from SoilMoist to SoilMoistFrac.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants