Sensitivity analysis MVP: MNPE/NPE posterior#729
Conversation
Builds on the MVP-1 foundation (#729) with categorical factor support, a cleaner analyzer/plotting separation, and a tighter eval-side / analysis-side contract that drops a class of drift bugs. - Analyzer hierarchy (BaseAnalyzer / PosteriorAnalyzer / NPEAnalyzer / MNPEAnalyzer / EmpiricalAnalyzer) dispatched via make_analyzer. Pure- categorical schemas use empirical frequency analysis directly (under uniform prior the posterior is exactly the normalized per-category success rate); sbi MNPE 0.26 also requires at least one continuous theta column, which this dispatch handles automatically. - Split inference (analyzer.py) from rendering (plotting.py). Analyzers expose continuous_marginal_density and categorical_marginal_probs queries; plotting consumes them via plot_marginal. New plot types become additive (free functions) without touching the analyzer. - Drop --factor_keys CLI flag on eval_runner. The writer now logs the full arena_env_args per episode; the analyzer-side factors.yaml picks what to study. Removes the drift bug class where --factor_keys and factors.yaml could disagree. - Rename JSONL field "factors" -> "arena_env_args". Honest about provenance and leaves room for sibling source fields (future "sim_state" for MVP-3 reset-time snapshots, "variation_draws" for the variation system) without further wire-format changes. - Add synthetic_data_categorical.py smoke-test generator and rename synthetic_data.py -> synthetic_data_continuous.py for symmetry. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
38baa56 to
74585f1
Compare
Adds a policy-sensitivity analysis stack under isaaclab_arena/analysis/ sensitivity/: a SensitivityDataset loader (factors.yaml + episode JSONL), NPE / MNPE / KDE / empirical analyzers (sbi-backed), continuous + categorical factor support with LogUniform priors, and an interactive Plotly HTML report. eval_runner gains an opt-in --episode_summary flag that appends one JSONL row per recorded episode (full arena_env_args dict + task outcomes); the analyzer decides which arena_env_args keys are factors via factors.yaml, so eval needs no knowledge of "factors". Job now carries arena_env_args_dict so the writer logs typed values. Adds sbi to dev deps. Driver scripts: analyze_sensitivity.py (single factor/outcome) and generate_sensitivity_report.py (full multi-factor HTML deliverable). Signed-off-by: Clemens Volk <cvolk@nvidia.com>
Paired (factors.yaml, jobs_config.json) sets for pi0 on the droid pick_and_place_maple_table task: * light_intensity_sweep — single continuous factor (light intensity) * pick_up_object_sweep — single categorical factor (object identity) * multi_factor_overnight_sweep — light_intensity (log-uniform) x 5 objects, num_episodes=4 * two_object_shiny_matte_sweep — focused 2-object contrast (matte mustard vs specular soup can) x log-uniform light, num_episodes=2 factors.yaml declares each factor's type/range/distribution for the analyzer; jobs configs are consumed by eval_runner --episode_summary. Use --chunk_size for the long sweeps to avoid host-RSS OOM. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
74585f1 to
48fba5d
Compare
main's metrics refactor (#733) made cfg.metrics a MetricsCfg configclass (one field per metric) rather than an iterable of metric objects. Iterate its fields and use compute_metric_func/recorder_term_name/params, matching MetricsManager. Fixes 'MetricsCfg object is not iterable' that produced empty episode-summary JSONL. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
sbi logs TensorBoard training curves under <cwd>/sbi-logs by default (get_log_root hardcodes the cwd), so fitting raised PermissionError when the cwd wasn't writable — e.g. generating a report from a repo checkout in a non-root container. A one-shot report fit never reads those curves, so pass a no-op tracker (_NullTracker) that discards them: no files written, no hidden cwd dependency, runs from any directory. Centralize the tracker on the base by having subclasses name their sbi class via _inference_cls. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
Grow the two-object light sweep to 500 rubiks_cube + 500 alphabet_soup_can (num_envs=2, num_episodes=2) for an overnight run with denser log-uniform light sampling. Update the factors.yaml header to match. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
Simplify the deliverable to one PDF on disk showing the most important plots (robolab-style): an outcome x factor grid of marginal-posterior plots, fit one analyzer per outcome. Drop the involved Plotly HTML report (report.py + its CLI) — to be reintroduced in a follow-up PR. - plotting.py: split the renderers into draw_marginal(ax, ...) that draws onto a caller-supplied Axes; plot_marginal keeps its single-figure save behavior. - pdf_report.py: new generate_pdf_report() lays out the grid and saves one PDF. - generate_sensitivity_report.py: now drives the PDF (--output_pdf). Signed-off-by: Clemens Volk <cvolk@nvidia.com>
Split the report title across two lines (report+episodes / slice) and widen the top margin so it doesn't clip when the grid is a single column. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
Scope PR #729 to the MVP: sensitivity to a single light_intensity factor on one object. Remove the multi-factor (multi_factor_overnight) and multi-object (two_object_shiny_matte, pick_up_object) sweep configs — including two 22k-line job configs — preserved on branch cvolk/feature/sensitivity_large_sweep_configs for the larger overnight runs. Keep only the single-factor light_intensity configs. Also drop the stale --factor_keys reference (no such flag; the writer logs the full arena_env_args). Signed-off-by: Clemens Volk <cvolk@nvidia.com>
Collapse the multi-line explanatory comment blocks to 1-2 dense lines, keeping the non-obvious 'why' (log-space transforms, the step-count/len gotcha, the deferred pxr import) and dropping the over-explanation. No code changes. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
Drop log_uniform from the first PR: the FactorSpec.distribution field, the YAML parsing/validation, the log10 theta transform + log-space prior in dataset, the log-grid handling in NPE/KDE marginals, and the log x-axis in plotting. The MVP sweeps linearly; analyzers and plotting are unchanged for linear factors. The full log_uniform implementation is preserved on branch cvolk/feature/sensitivity_log_uniform for a follow-up PR. Verified post-removal: KDE, MNPE+categorical, and NPE all fit and render. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
…tracker - Remove the verbose module-level docstrings across the sensitivity package; the two synthetic-data generators and the two CLI scripts now pass a short literal argparse description instead of `description=__doc__`. - Remove the `_NullTracker` workaround. With in-container runs no longer executing as root, sbi's default tracker writes a (gitignored) `sbi-logs/` owned by the user, so the PermissionError it guarded against no longer occurs. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
…nalyzers - Make `EmpiricalAnalyzer` an abstract base for the two direct (non-neural) analyzers, which estimate the posterior straight from data under a uniform prior. Rename the concrete categorical analyzer to `FrequencyTableAnalyzer` and reparent `KDEAnalyzer` beneath the same base. Both now share a named `SUCCESS_THRESHOLD` and a `_success_mask()` helper instead of inlining the `>= 0.5` success test. - Update `make_analyzer` dispatch and the plotting/docstring references; fix a stale claim that only `PosteriorAnalyzer` provides `continuous_marginal_density` (KDEAnalyzer does too). - Drop `v0.3`/`MVP-1` wording from the analyzer docstrings, keeping the substantive uniform-prior and binary-outcome assumptions. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
- Break the monolithic analyzer.py into analyzer_base, posterior_analyzer, and empirical_analyzer modules along the neural/empirical family seam. - Move the make_analyzer dispatch into factory.py, re-exported from the package __init__; lazy concrete imports keep package import free of torch/sbi so the eval-time episode_writer path stays light. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
They're standalone smoke-test tools — not part of the runtime pipeline — so they don't belong in the production analysis namespace. Relocated to the test-helper package, ready to back a sim-free analyzer regression test. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
Relocate analyze_sensitivity.py / generate_sensitivity_report.py from scripts/ into analysis/sensitivity/ as analyze.py / generate_report.py, mirroring how eval_runner/policy_runner live flat inside the evaluation package. Drops the redundant "sensitivity" prefix now that the package name carries it. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
- Ship one analyzer per family — KDEAnalyzer (empirical) and MNPEAnalyzer (the sbi robolab port) — keeping the reviewable surface small while still demonstrating the multi-analyzer design across both the empirical and neural families. - Park NPEAnalyzer, FrequencyTableAnalyzer, and the now-orphaned categorical synthetic data generator on cvolk/feature/sensitivity_deferred_analyzers to bring in later. - Guard the deferred factor mixes in make_analyzer with clear asserts pointing at that branch: pure-categorical → FrequencyTable; multi-continuous or non-binary → NPE. - Keep the PosteriorAnalyzer/EmpiricalAnalyzer family bases as the extension points the parked siblings re-attach to; drop the now-unused binary-outcome warn hook. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
Drop git-branch references and the "robolab" attribution from the make_analyzer asserts and the analyzer docstrings; state the unsupported factor mixes plainly. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
episode_writer is eval-time data production — it runs inside the eval loop, depends on the metrics/evaluation machinery, and is called only by eval_runner. It has no coupling to the analysis code (the analyzer consumes the JSONL purely as a format). Relocating it beside its caller frees the sensitivity package of any pxr/sim dependency. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
The analyze.py CLI rendered one (factor, outcome) marginal to a PNG — a strict subset of the outcome × factor grid that generate_report already produces. Remove it along with the now-unused plot_marginal/_plot_title/_save_figure helpers, leaving draw_marginal (used by the PDF report) as the single rendering path. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
task_duration was a synthetic-only continuous outcome (no matching registered metric) that existed to exercise NPE. With NPE deferred, a single continuous factor with a non-binary outcome now asserts, so emitting it made generate_report crash on the smoke dataset. The generator now emits only the binary success/object-moved outcomes the MVP analyzes. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
Address PR review: change the copyright headers on the sensitivity package files (plus the moved episode_writer and the synthetic generator) from 2025-2026 to 2026, matching the --use-current-year convention used by new files in the repo. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
'rich' wasn't descriptive and duplicated the MNPE case. Fold it into make_mixed_dataset
(now 3 continuous + 2 categorical — 'mixed' = mixed factor types) and drop make_rich_dataset.
make_continuous_dataset stays for the NPE path. Two builders, one per estimator, both named
for what they are. The MNPE test now asserts all five planted effects; --kind is {mixed, continuous}.
Signed-off-by: Clemens Volk <cvolk@nvidia.com>
|
/review |
There was a problem hiding this comment.
🤖 Isaac Lab-Arena Review Bot
Summary
Adds a self-contained sensitivity-analysis toolbox (isaaclab_arena/analysis/sensitivity/) that fits an sbi neural posterior (MNPE/NPE) over swept factors conditioned on an outcome and renders one marginals figure. The layering is clean — it's a CPU-only analysis layer over eval JSONL with no sim coupling — the typed FactorSpec/FactorSchema design is nice, and the synthetic-ground-truth tests cover both estimator paths. A couple of points below worth resolving before merge.
Design, Boundaries & Scope
The toolbox is an optional, narrowly-used MVP, but sbi (plus scipy, matplotlib) is now in the core RUNTIME_DEPS, so every install of isaaclab_arena pulls the full SBI stack (pyro/pyknos/…) whether or not anyone runs a report. Per Arena's lean-by-default / conservative-defaults stance, would it be better to make the whole analysis.sensitivity subpackage an optional extra (extras_require={"analysis": ["sbi", "scipy", "matplotlib"]}) and defer the top-level sbi/matplotlib/scipy imports so the core stays lean and an analysis-free install doesn't carry the weight? See the inline note on setup.py.
Findings
🟡 Warning: analyzer.py:73 / generate_report.py — The report path is not seeded, so it isn't reproducible.
🔵 Improvement: dataset.py:220 — SensitivityDataset.prior (and its BoxUniform import) appears to be dead code.
🟡 Warning: setup.py:20 — heavy sbi dependency added to core runtime deps (see Design section).
Test Coverage
Good. test_sensitivity_analysis.py builds in-memory synthetic datasets with a known planted relationship and asserts the posterior recovers it, covering both the MNPE (mixed) and NPE (continuous-only) paths with substantive statistical assertions. These are pure CPU tests with no Isaac Sim dependency, so the inner/outer run_simulation_app_function pattern correctly does not apply and they land in Phase 1. One thing to confirm: the tests set torch.manual_seed(0) before fit(), so they rely on sbi training being deterministic under that seed — if it proves version-sensitive in CI, the statistical assertions could become flaky.
Verdict
Minor fixes needed
There was a problem hiding this comment.
🤖 Isaac Lab-Arena Review Bot
Summary
Adds a self-contained sensitivity-analysis toolbox (isaaclab_arena/analysis/sensitivity/) that fits an MNPE/NPE posterior over swept factors conditioned on an outcome and renders a marginals figure, plus a synthetic ground-truth simulator and end-to-end tests. The module is cleanly layered — pure-Python, sim-agnostic, no coupling into the Scene/Embodiment/Task primitives — and the synthetic-data tests are a nice way to validate recovery of planted effects on CPU. My main concern is the dependency footprint added to the core install; the rest are small cleanups.
Design, Boundaries & Scope
The one scope concern is setup.py: sbi, scipy, and matplotlib are added to the core RUNTIME_DEPS, so every Arena install now pulls them in (sbi in particular drags in a large stack: pyro, nflows/zuko, etc.) for what is an opt-in analysis MVP that nothing in the core import path touches. extras_require already exists (dev). Could these move to an analysis extra so the default install stays lean? See the inline note.
Findings
See inline comments. In short: heavy deps added to the core install (🟡), an unused prior property (🔵), full estimator training in the default test phase (🔵), and docs describing an eval-runner integration that doesn't exist yet (🔵).
Test Coverage
Good: both estimator paths (MNPE for the mixed schema, NPE for continuous-only) are exercised against a known ground truth with substantive assertions on the recovered posterior, and seeds are set for reproducibility. No sim is involved, so the inner/outer run_simulation_app_function pattern correctly doesn't apply and the unmarked tests land in Phase 1 as intended. One caveat on test cost noted inline.
Verdict
Minor fixes needed
alexmillane
left a comment
There was a problem hiding this comment.
Great work. Super clean.
I have a bunch of nits, but merge away when you see fit.
SensitivityAnalyzer.__init__ indexed factor.range[0] directly, so a continuous factor with range=None (e.g. a dataset built via the constructor rather than from_files, where the range is never inferred) raised an opaque TypeError. Add an assert with a message pointing at how to supply the range. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
generate_report() trains the estimator and samples the posterior, both of which consume torch's global RNG, but nothing seeded it — so report output varied run-to-run. Seed torch once before fitting (new --seed, default 0; pass None to leave the RNG untouched). Signed-off-by: Clemens Volk <cvolk@nvidia.com>
The analyzer builds its own _normalized_prior() over the normalized [0,1] space and never reads dataset.prior; nothing else referenced it. Remove the property and the now-orphaned BoxUniform import. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
The existing tests only build datasets in memory; nothing covered the file parsing path. Add two parse-only tests for SensitivityDataset.from_files: one asserting the mixed-schema theta/x layout (continuous-first, categorical integer-coded by choice index) and outcome casting, and one asserting that a continuous factor with no declared range gets [min, max] inferred from the data. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
Replace the Literal["continuous", "categorical"] with a FactorType(str, Enum) plus __post_init__ coercion, giving a single source of truth for the valid values, fail-fast at construction, and autocomplete. Being a str-Enum, it compares equal to its string value, so the existing == "continuous" comparisons and type="continuous" constructions keep working unchanged. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
Each range entry is a fixed (low, high) pair, so list[tuple[float, float]] documents the arity better than list[list[float]]. __post_init__ coerces the lists that YAML/JSON deliver to tuples (and the range-inference path emits a tuple) so the annotation matches the runtime values. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
synthetic.py is test infrastructure (planted-ground-truth generators the unit tests assert against) plus a convenience CLI, not part of the shipped analysis.sensitivity API. Move it to isaaclab_arena/tests/sensitivity_synthetic.py, next to its only consumer, and update the test import and the python -m paths in its docstring and the docs. Keeps it off the core API surface without making the tests depend on the examples package. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
State that the schema describes what can vary, not the per-episode values, and drop the wordy outcome-conditioning aside (per review). Signed-off-by: Clemens Volk <cvolk@nvidia.com>
State sbi's convention explicitly: theta is the per-episode factor values the posterior is inferred over, x is the per-episode outcomes a query conditions on. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
Address review nits on the sensitivity concept page: give an example outcome at first mention (success rate), call the continuous-factor plot a probability density curve, and drop the CPU aside from the synthetic-data section. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
Add an intro paragraph stating what the posterior is (the prior over swept factor values reweighted by how often each led to the chosen outcome) and separating the two ideas a reviewer asked about: joint (captures interactions/confounds) vs posterior (conditions on the outcome). Signed-off-by: Clemens Volk <cvolk@nvidia.com>
The eval-side writer (episode_writer / episode_summary.jsonl) is not part of this version. Drop the present-tense claims that it records during evaluation, and add a TODO noting it lands in a follow-up and that this version runs on synthetic data or an externally-produced JSONL. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
Introduce the terms at first use in the Inference step: theta is the factor values, x the per-episode outcomes (sbi's terms). Signed-off-by: Clemens Volk <cvolk@nvidia.com>
Add the precise reading (among successful episodes, the probability density that the factor took each value) alongside the intuitive 'which values were responsible', and drop the unexplained 'light-gated' jargon. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
Extend the Current scope vector-factor bullet: when dim>1 factors land, the plan is to record scalar reductions (norm / distance-to-reference) alongside the raw vector so a pose or RGB factor becomes analysable scalar columns. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
_normalize/_denormalize slice theta[:, :self._num_continuous] assuming continuous factors lead. Add a comment noting that layout is established by SensitivityDataset and FactorSchema.factor_columns, so the invariant is documented at the slice. Signed-off-by: Clemens Volk <cvolk@nvidia.com>
Summary
Sensitivity analysis toolbox (MNPE/NPE) running on synthetic data. From an eval sweep's per-episode results, learn which environment conditions drive success: Fit a posterior over the varied factors, conditioned on the outcome, and render one summary figure.
Inspired by robolab, but the factors now come from a factors.yaml and are piped generically, allowing for arbitrary continuous/categorical factor mixes.
Detailed description
factors.yamldeclares the varied factors and ranges. This file will be auto generated by the Variation System and could be moved into one time write into the same output file. For now its hand crafted.SensitivitDatasetgenericatlly handling n dimensional factors, categorical and continousSensitivityAnalyzerauto-selects MNPE (mixed continuous + categorical) or NPE (continuous-only), trains on the full(theta, x), and samples the joint posterior at a chosen observation. Continuous factors are normalized so factors on very different scales train on equal footing.generate_reportproduces one figure, a density curve per continuous factor, a probability bar per categorical, saved by file extension (.png/.pdf).A synthetic ground-truth simulator (
synthetic.py)python -m isaaclab_arena.analysis.sensitivity.synthetic --kind {mixed,continuous,rich}runs the whole pipeline in one command.Next: Plug in real sim/ variation pipeline