Skip to content

feat(uncertainty): outdir opt-in for get.parameter.samples wrapper (Modularity Part 3)#4016

Merged
mdietze merged 1 commit into
PecanProject:developfrom
omkarrr2533:gsoc/parameter-samples-outdir
Jun 1, 2026
Merged

feat(uncertainty): outdir opt-in for get.parameter.samples wrapper (Modularity Part 3)#4016
mdietze merged 1 commit into
PecanProject:developfrom
omkarrr2533:gsoc/parameter-samples-outdir

Conversation

@omkarrr2533
Copy link
Copy Markdown
Member

@omkarrr2533 omkarrr2533 commented May 30, 2026

Third PR in the trait → meta-analysis → parameter-sampling modularity series (after #3860 Part 1, #4010 Part 2). Cleans up the get.parameter.samples() wrapper now that the pure get_parameter_samples() from #3860 does the computation.

What changed

Backward compatibility

No behavior change for existing callers — none of them pass the new argument, so outdir defaults to settings$outdir and samples.Rdata is written exactly as before. The only removal is save_to_disk: grepped the repo, it appears only inside get.parameter.samples.R and nothing passes it, so dropping it is safe (it was never in a release).

Per the Slack discussion with @mdietze and @infotroph: nothing changes for disk-writing workflows in this PR. The ensemble.samples.*.Rdata / sensitivity.samples.*.Rdata files that workflows like the SIPNET segmented restart (#3919 / #4008) rely on are written by run.write.configs(), which this PR does not touch, and samples.Rdata still writes by default. Migrating the generate_*_design() and SDA callers to the dash function is deferred to Phase 2/3.

Tests

Adds test-get.parameter.samples.R in the correct package path (modules/uncertainty/): default save to settings$outdir, explicit outdir override, outdir = NULL skip, the 5-object samples.Rdata name contract, invisible return, and delegation to get_parameter_samples(). load.posteriors() and get_parameter_samples() are stubbed with mockery, so it runs with no database. Local run for this file: [ FAIL 0 | SKIP 0 | PASS 10 ].

Also relocates the misplaced test-get.parameter.samples.R under modules/meta.analysis/tests/testthat/ (added in #3858): renamed to test-p.point.in.prior.R since its remaining tests cover the PEcAn.MA helper p.point.in.prior(), and dropped the obsolete get.parameter.samples skip-test (stale since #3860, superseded by the test added here).

@omkarrr2533 omkarrr2533 marked this pull request as ready for review May 31, 2026 04:28
@S1DDHEY
Copy link
Copy Markdown
Contributor

S1DDHEY commented May 31, 2026

One note on the description: it says the wrapper “previously returned NULL invisibly”, but on current develop get.parameter.samples() already ends with invisible(result) (returns the list invisibly). In this PR, I only see the return behavior being preserved, not changed.

If you meant “it still returns invisibly (not NULL)”, could you please update the PR description accordingly? Or point to the commit/branch state where it was returning NULL so the statement matches history.

Also currently there is another test-get.parameter.samples.R under modules/meta.analysis/tests/testthat/ which firstly is in the wrong place (added in #3858) and also is not needed anymore as you've added one here with the correct path in this PR, would suggest removing it.

Replace the save_to_disk flag from PecanProject#3860 with an outdir parameter (default settings$outdir): samples.Rdata is saved when outdir is non-NULL and skipped when outdir = NULL. Existing callers keep writing to settings$outdir unchanged. The wrapper stays deprecated, delegates to get_parameter_samples(), and now returns the result list invisibly instead of NULL. Adds wrapper-level tests for outdir behaviour, the 5-object samples.Rdata contract, invisible return, and delegation.
@omkarrr2533 omkarrr2533 force-pushed the gsoc/parameter-samples-outdir branch from 005e21c to 23ca6c0 Compare May 31, 2026 15:45
@omkarrr2533
Copy link
Copy Markdown
Member Author

thanks @S1DDHEY.
On the return you're right, that line was misleading. The switch from invisible(NULL) to returning the list happened back in #3860; this PR doesn't change the return, just preserves it and adds a test to lock the contract in. Updated the description.
On the misplaced test looked at it, and that file's a mix: the get.parameter.samples test in there was just a skip() placeholder for the old behaviour, but the same file also holds the p.point.in.prior tests, which are real PEcAn.MA helper tests. So rather than delete it I renamed it to test-p.point.in.prior.R and dropped the obsolete skip-test clears the misplaced get.parameter.samples test without losing the prior coverage.

@mdietze mdietze added this pull request to the merge queue Jun 1, 2026
Merged via the queue into PecanProject:develop with commit 6aec657 Jun 1, 2026
19 of 25 checks passed
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