Skip to content

support SIPNET sensitivity analysis across crop changes - #4136

Open
divine7022 wants to merge 23 commits into
PecanProject:developfrom
divine7022:sa-site-pfts
Open

divine7022 wants to merge 23 commits into
PecanProject:developfrom
divine7022:sa-site-pfts

Conversation

@divine7022

@divine7022 divine7022 commented Sep 28, 2026 •

Copy link
Copy Markdown
Member

Description

This replaces the initial PFT filter with support for sensitivity analysis across SIPNET crop changes. site.pft still identifies the initial vegetation. Crops introduced later use their sampled parameters when active and remain
in the sensitivity results.

It also fixes baseline only manifests and selects constants for correct PFT, while preserving NULL defaults for existing callers. Direct run.write.configs calls accept one analysis at a time; combined SA and ensemble runs use runModule.run.write.configs.

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.

Comment thread base/workflow/R/run.write.configs.R Outdated
# a site's configs only get its own PFTs' parameters (see
# write.ensemble.configs), so a run moving another PFT's trait would just
# repeat the median run
site.pfts <- intersect(as.character(unlist(settings$run$site$site.pft)), pft.names)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does SA support segmented runs? In the segmented case site.pft contains the initial PFT and others are activated in later segments using the PFT parameters passed here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suspect one could make a design argument that from the run.write.configs perspective maybe site.pft should contain every PFT the site will use at any time in the run. But making it work that way would need a change in how write.config.SIPNET handles it and I'm not immediately sure how we'd mark the initially active PFT.

@infotroph

Copy link
Copy Markdown
Member

@divine7022 gentle nudge about my question above -- seems important to get right before merging.

@divine7022

Copy link
Copy Markdown
Member Author

Thanks Chris!. my recent SA runs were unsegmented, and I haven’t tested segmented SA. The current segmented writer expects ENS run IDs and ensemble.samples files, so I haven’t established working segmented SA path

I see your point about site.pft, for a segmented run it can identify the initial vegetation, while another PFT is activated later. my filter and documentation wording assume it is full set of PFTs used by run.

Would it make sense to keep this PR scoped to fixed PFT SA rather than redefine site.pft here ?

While checking this, I also found two separate bugs in the patch. A combined SA and ensemble call reuses the filtered design, and median only design fails while building the manifest. I’ll fix both and add tests before asking for another review

@divine7022 divine7022 changed the title write sensitivity runs only for a site's own PFTs support SIPNET sensitivity analysis across crop changes Nov 8, 2026
expect_error(sipnet_pft_constants(defaults, "cropA"), "unique names")
})

test_that("generic ensemble and SA writers accept the auxiliary environment group", {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Heads up that env will go away in #4229, at which point this test will probably need rethinking

@infotroph

Copy link
Copy Markdown
Member

@divine7022

current segmented writer expects ENS run IDs and ensemble.samples files

Ah right, that would definitely complicate using it in SA 🫤. Yes, OK to keep this PR scoped to fixed PFTs.

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.

3 participants