Add generate_OAT_SA_design() for sensitivity analysis input design by divine7022 · Pull Request #3729 · PecanProject/pecan

divine7022 · 2025-12-22T22:16:31Z

originally discussed -- comment

Description

Add generate_OAT_SA_design() function to create input design matrices for OAT sensitivity analysis.
SA requires isolating the effect of each parameter by holding all other inputs (met, IC, soil) constant. The existing generate_joint_ensemble_design() randomizes these inputs, which is correct for ensemble runs but invalidates SA variance decomposition.

added tests in test.input_design.R to validate the OAT sensitivity design, including checks :

that the total number of runs is calculated correctly
all non-parameter inputs (such as met, IC, and soil data) remain constant across OAT steps as required for valid SA
comparison test showing the key difference from ensemble design

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.

mdietze

A good overall test that this function is working correctly would be to verify that the run.write.configs module runs correctly with the OAT design as an input and continues to generate output that's readable by the SA postprocessing and graphing functions. If it doesn't, then this function is generating inputs that are not serving any useful purpose. Along the way, we may want/get to simplify the run write configs logic

mdietze · 2025-12-24T15:34:22Z

+#' all inputs vary together.
+#'
+#' @param settings PEcAn settings object
+#' @param sa_samples Optional. Pre-loaded SA samples from samples.Rdata.


This argument suggests a fundamental design misunderstanding. sa_samples should be generated by this function, not read by it. I'm OK with taking this as an optional input if one wants to reuse a previous sa_samples, but if it's not provided it needs to be generated

the parameter now clearly indicates it's optional for reusing existing samples, not the expected input

mdietze · 2025-12-24T15:40:49Z

+  if (is.null(sa_samples)) {
+    samples_file <- file.path(settings$outdir, "samples.Rdata")
+
+    # generate samples if they don't exist (safety fallback)


This should be the default, not the safety fallback

removed "safety fallback" comment

mdietze · 2025-12-24T15:41:47Z

+
+      PEcAn.uncertainty::get.parameter.samples(
+        settings,
+        ensemble.size = 1,  # SA doesn't need ensemble samples


not needed as this is the default

removed, good catch!

mdietze · 2025-12-24T15:42:07Z

+        settings,
+        ensemble.size = 1,  # SA doesn't need ensemble samples
+        posterior.files,
+        ens.sample.method


also not needed as this isn't an ensemble sample

mdietze · 2025-12-24T15:51:27Z

+#' @export
+#' @author Akash B V
+#' @importFrom rlang %||%
+generate_OAT_SA_design <- function(settings, sa_samples = NULL) {


It might be too much at this PR, but it would be good to move away from settings as a dependency unless the underlying number of pieces of information is too large to meaningfully pass to the function. But in that case it would be good to document exactly what part of the settings is required. Here I think it might just be settings$outdir, settings$pfts, settings$sensitivity.analysis, and settings$ensemble (i.e. I wonder if you could get away with a function that has outdir, pfts, sensitivity.analysis, ensemble, and sa_samples as arguments?). Would also be good to better document semi-hidden dependencies (e.g. what does the pft$posterior.files need to point to for the function to actually sample parameters correctly)

I agree with reducing dependency of settings for input desing by passing specific argument, but after analyzing both functions to keep the architecture consistent, i found that the parameter requirements are not same; generate_OAT_SA_design needs outdir, pfts, samplingspace, sensitivity.analysis and generate_joint_ensemble_design additionally needs run$inputs (via input.ens.gen() which samples from settings$run$inputs[[input]]$path).
However there is a deeper blocker -- both functions call get.parameter.samples(settings, ...) which itself uses many settings fields (database$bety, host$name, sensitivity.analysis, etc.). So even with explicit parameters in the design functions, we'd still pass settings through to get.parameter.samples. And then that involves refactoring SDA and sobol callers.
anyways i have documented what setting it uses and semi-hidden dependiences in both desing function. I happy to know ur thoughts.

I think it's fine if the parameters requirements are not the same. I also think it's fine to push the refactor of the generate design functions and get.parameter.samples to a future PR

mdietze · 2025-12-26T19:09:38Z

── Failed tests ────────────────────────────────────────────────────────────────
Error ('test.input_design.R:165:3'): OAT design integrates with write.sa.configs for SA postprocessing
Error in PEcAn.uncertainty::write.sa.configs(defaults = settings$pfts, quantile.samples = sa_samples, settings = settings, model = "FAKE", write.to.db = FALSE, input_design = input_design): unused argument (input_design = input_design)

divine7022 · 2025-12-26T19:21:44Z

PEcAn.uncertainty::write.sa.configs(defaults = settings$pfts, quantile.samples = sa_samples, settings = settings, model = "FAKE", write.to.db = FALSE, input_design = input_design): unused argument (input_design = input_design)

we are passing input_design as a new parameter to write.sa.configs() in #3708 -- all tests are passed when i branched that PR.
This will fix when we merge #3708

infotroph · 2026-01-07T17:23:41Z

+#' one-at-a-time across quantiles. This differs from ensemble design where
+#' all inputs vary together.
+#'
+#' @param settings PEcAn settings object. This function directly uses:


Consider "see details" and moving the itemized list down? I find this long a @param entry makes it hard to skim all the options.

infotroph · 2026-01-07T17:32:12Z

+make_mock_sa_samples <- function() {
+  list(
+    pft1 = structure(
+      matrix(1:9, nrow = 3, ncol = 3),
+      dimnames = list(c("25", "50", "75"), c("trait1", "trait2", "trait3"))
+    )
+  )
+}


Returns a constant -- why is this a function and not just a list?

infotroph · 2026-01-07T17:41:23Z

@@ -0,0 +1,194 @@
+make_sa_test_settings <- function() {
+  list(
+    outdir = withr::local_tempdir(),


This path looks to me as if it will stop existing when make_sa_test_settings returns! Is the outdir used for anything during testing or could this just be "/fake/output/path/" and skip the withr usage?

thanks, yes never accessed, when i see the settings parameter requirements for generate_OAT_SA_desing;
used "/fake/output/path/" and commented

infotroph · 2026-01-07T19:05:42Z

+    expect_true(all(result$X[[col]] == 1),
+      info = paste("Column", col, "should be constant 1 for SA"))


?expect_true discourages using info in new code, and the expectation seems pretty clear without it to me. Could unquote for slightly better diagnostic messages:

Suggested change

expect_true(all(result$X[[col]] == 1),

info = paste("Column", col, "should be constant 1 for SA"))

# all columns but `param` should be a constant 1

# (`!!` to get the column name into the failure message)

expect_true(all(result$X[[!!col]] == 1))

infotroph · 2026-01-07T19:25:06Z

+})
+
+test_that("generate_OAT_SA_design param column is sequential", {
+  settings <- make_sa_test_settings()
+  sa_samples <- make_mock_sa_samples()
+
+  result <- generate_OAT_SA_design(settings, sa_samples = sa_samples)


Style nit: Since generating the design takes several lines of setup and is identical for both these tests, multiple expectations in the same test block feels cleaner to me.

Suggested change

})

test_that("generate_OAT_SA_design param column is sequential", {

settings <- make_sa_test_settings()

sa_samples <- make_mock_sa_samples()

result <- generate_OAT_SA_design(settings, sa_samples = sa_samples)

(If you accept this, probably want to edit line 38 to something like "...keeps param column sequential and others constant at 1")

yup, unified test "generate_OAT_SA_design keeps param sequential and non-param constant at 1"

infotroph · 2026-01-07T19:35:16Z

@@ -0,0 +1,194 @@
+make_sa_test_settings <- function() {


Please match test file names to R file names by naming this file test-generate_OAT_SA_design.R, to avoid needing to wonder which test file to look in to go along with the code you're writing (or vise versa).

the file tests both generate_OAT_SA_design and generate_joint_ensemble_design (plus their comparison, etc), naming it test-generate_OAT_SA_design.R would suggest it only tests SA design. so i kept test.input_design.R to reflect that it covers both design types

Then please move the tests of generate_joint_ensemble_design to their own file, and if there are shared helpers then put them in a file whose name starts with helpers.

See https://r-pkgs.org/testing-basics.html for more on recommended organization, but know that the reason I'm insisting on this is that unit testing is built on boring consistency, and inconsistent test file names will confuse future maintainers (including me).

thanks for pointing this out, agreed!

infotroph · 2026-01-07T19:39:30Z

+  sa_non_param <- setdiff(names(sa_result$X), "param")
+  for (col in sa_non_param) {
+    expect_equal(length(unique(sa_result$X[[col]])), 1,
+      info = "SA design: non-param columns must be constant")
+  }


Isn't this the same condition you already tested around line 45?

yeah, this is intentional, to check in the comparison test of ens and SA design ( now simplified and with clear intent )

infotroph · 2026-01-07T19:48:54Z

+      info = "SA design: non-param columns must be constant")
+  }
+
+  ## ensemble design - non-param can vary (mocked to show variation)


I don't follow what "to show variation" means here and why it needs all the mocks

the mock returns varied indices c(1, 2, 3, 1, 2) to demonstrate that the ensemble design structure passes through whatever input.ens.gen returns, unlike the OAT design which forces all non-param columns to constant

and now made the comment much clear

infotroph · 2026-01-07T20:06:54Z

+  for (run_id in run_ids) {
+    expect_true(dir.exists(file.path(rundir, run_id)))
+  }
+})


Suggested change

})

})

infotroph · 2026-01-07T20:16:03Z

+#------------------ tests: OAT design with write.sa.configs -------------------
+# verifies design produces output compatible with SA postprocessing
+
+test_that("OAT design integrates with write.sa.configs for SA postprocessing", {


The degree of complication needed to set up these tests feels like an indicator that we could do better at designing these functions for testability, but I think that can be a future project.

… generate-sa-design

divine7022 · 2026-01-08T11:11:50Z

@infotroph please take a look, once you approved i will merge this branch with #3708
since both PRs the depended on merging each other

divine7022 · 2026-02-23T19:26:51Z

heads-up : generate-sa-design branch is already merged with run-manifest #3708

leaving this PR open until we merge run-manifest branch

divine7022 added 9 commits December 22, 2025 21:50

function for sa specific desing

6810331

update .Rd

41c5d99

tests for input design

162e6ad

update changelog

875a7f5

update NEWS.md

932f0ab

update NAMESPACE

2a8ac94

update comment

e2f3dd9

update roxy

f2e8193

update .Rd

ab3dcc7

github-actions Bot added tests modules labels Dec 22, 2025

divine7022 added 3 commits December 22, 2025 22:20

updade CHANGELOG.md

6b4c4dd

update NEWS.md

0fbf2cd

removed comment

2bc446b

divine7022 requested review from dlebauer, infotroph and mdietze December 22, 2025 22:24

mdietze reviewed Dec 24, 2025

View reviewed changes

divine7022 added 8 commits December 24, 2025 21:11

fix SA design to generate samples by default

24a608f

verifies OAT design with SA post processing

7c65f43

update roxy

7f5b3d7

update roxy

de90a59

update generate_OAT_SA_design.Rd

f75e64e

update generate_joint_ensemble_design.Rd

f22c103

add withr to DESCRIPTION

8574b58

add withr to docker depends

9c6fc58

github-actions Bot added the dockerfile label Dec 25, 2025

mdietze approved these changes Dec 26, 2025

View reviewed changes

Merge remote-tracking branch 'origin/develop' into generate-sa-design

ac5519c

This comment was marked as resolved.

Sign in to view

This was referenced Dec 31, 2025

Refactor workflow to stateless run manifest architecture and SA input design coordination #3708

Merged

CI failure in check_modules( R 4.2) #3750

Merged

Merge branch 'develop' into generate-sa-design

324c3ac

dlebauer enabled auto-merge January 6, 2026 21:10

Merge branch 'develop' into generate-sa-design

9adabe2

infotroph disabled auto-merge January 7, 2026 06:01

infotroph requested changes Jan 7, 2026

View reviewed changes

divine7022 added 5 commits January 8, 2026 09:56

migrate settings requirements from param to details

92abd6e

update test

b4577bb

update .Rd

a73f498

Merge remote-tracking branch 'origin/develop' into generate-sa-design

a6795c8

Merge branch 'generate-sa-design' of github.com:divine7022/pecan into…

237b2e2

… generate-sa-design

divine7022 requested a review from infotroph January 8, 2026 11:08

divine7022 added 4 commits January 8, 2026 18:51

shared test fixtures for input design tests

8115981

migrate SA desing test to seperate test-generate_OAT_SA_design.R

01d2ed4

migrate ens desing to seperate test-generate_joint_ensemble_design.R

112efb2

remove combined SA and ens desing test file - test.input_design.R

f523f15

dlebauer added the ccmmf issues and pre related to the ccmmf project label Jan 20, 2026

github-merge-queue Bot closed this pull request by merging all changes into PecanProject:develop in dab62a6 Feb 26, 2026

		expect_true(all(result$X[[col]] == 1),
		info = paste("Column", col, "should be constant 1 for SA"))

-    expect_true(all(result$X[[col]] == 1),
-      info = paste("Column", col, "should be constant 1 for SA"))
+   # all columns but `param` should be a constant 1
+   # (`!!` to get the column name into the failure message)
+    expect_true(all(result$X[[!!col]] == 1))

Conversation

divine7022 commented Dec 22, 2025

Description

Motivation and Context

Review Time Estimate

Types of changes

Checklist:

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!

mdietze commented Dec 26, 2025

Uh oh!

divine7022 commented Dec 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

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!

divine7022 commented Jan 8, 2026

Uh oh!

divine7022 commented Feb 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

divine7022 commented Dec 26, 2025 •

edited

Loading