Sensitivity analysis MVP: MNPE/NPE posterior

[cvolkcvolk]

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.yaml declares 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.

• SensitivitDataset genericatlly handling n dimensional factors, categorical and continous

• SensitivityAnalyzer auto-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_report produces 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

[greptile-apps]

factor.range indexed without a None guard in __init__

Both list comprehensions on lines 38–39 call factor.range[0][0] and factor.range[0][1] directly. FactorSpec.range defaults to None, and the in-memory constructor path (SensitivityDataset(schema, theta, x)) has no assertion that continuous factors carry a range before SensitivityAnalyzer is constructed. If a user builds a schema in-memory with FactorSpec(name="x", type="continuous") (omitting range), the constructor crashes with TypeError: 'NoneType' object is not subscriptable rather than a legible assertion. The from_files path is safe because _infer_missing_factor_ranges fills the range first, but the in-memory path has no equivalent guard here.

[cvolkcvolk]

Note to only add a TODO, this is not yet in this version

[cvolkcvolk]

/review

[arena-review-bot]

🤖 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

[arena-review-bot]

🟡 Reproducibility: fit() trains a neural estimator and sample_posterior() draws 5000 samples — both consume the global torch RNG, but nothing in the report path seeds it. The tests only pass because they call torch.manual_seed(0) themselves; generate_report(...) and the synthetic.py --kind ... CLI set no seed, so running the same episode_summary.jsonl twice yields different posteriors and a different figure. For an eval/benchmark library that's a reproducibility gap.

Could we thread a seed through generate_report / the CLI (and ideally the analyzer) and set it via the existing isaaclab_arena.utils.random.set_seed, plus log the seed used so a run can be reproduced?

[arena-review-bot]

🔵 This prior property doesn't seem to be used anywhere — SensitivityAnalyzer builds its own _normalized_prior() (over the normalized [0,1] continuous dims) and never reads dataset.prior. BoxUniform is imported (line 15) only for this property. Is it needed, or can the property and the import be dropped?

[alexmillane]

@cvolkcvolk could be true?

[arena-review-bot]

Yep — I traced it again to be sure: the only plausible reader is the analyzer, and it builds _normalized_prior() itself rather than calling dataset.prior. So both the property and the BoxUniform import look safe to drop unless you're planning to wire it in soon. Happy to be wrong if there's a downstream consumer I missed.

[arena-review-bot]

🟡 This puts the full sbi stack (pyro, pyknos, …), plus scipy/matplotlib, into the core RUNTIME_DEPS, so every isaaclab_arena install pulls it even when no one runs a sensitivity report. Since analysis.sensitivity is an optional MVP, would an extra be cleaner?

Suggested change

	"sbi",
	"openai>=2.0",

...and move these to extras_require (e.g. "analysis": ["sbi", "scipy", "matplotlib"]), making the top-level imports in the analysis.sensitivity modules lazy so an analysis-free install stays lean. What do you think?

[cvolkcvolk]

We would want to run the sensitivity analysis in one go witht the simulation runs eventually

[arena-review-bot]

Concern still stands, I think: as written every isaaclab_arena install pulls the full sbi stack + scipy/matplotlib just to import the package, even for users who never touch sensitivity analysis. Concretely: move sbi/scipy/matplotlib to extras_require under an "analysis" key and make the top-level imports in analysis.sensitivity lazy, so an analysis-free install stays lean and pip install isaaclab_arena[analysis] opts in. Happy to be wrong if there's a reason these need to be core — is there?

[cvolkcvolk]

test

[arena-review-bot]

Got it (test received 👍). Whenever you have a moment — any thoughts on moving the sbi/scipy/matplotlib stack into an analysis extra and making the analysis.sensitivity imports lazy, so an analysis-free install stays lean?

[arena-review-bot]

🤖 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

[arena-review-bot]

🟡 Warning — this puts sbi, scipy, and matplotlib in the core RUNTIME_DEPS, so every isaaclab_arena install now pulls them in. sbi especially is heavy (it drags in pyro, nflows/zuko, a large ML stack) and nothing in the core import path imports analysis.sensitivity — it's an opt-in MVP. Could we keep the default install lean by making this an extra instead? There's already an extras_require block to extend:

    extras_require={
        "dev": DEV_DEPS,
        "analysis": ["sbi", "scipy", "matplotlib"],
    },

Then pip install isaaclab_arena[analysis] enables the toolbox without taxing everyone else.

[arena-review-bot]

🔵 Improvement — this prior property doesn't appear to be used anywhere: the analyzer builds its own _normalized_prior() (it needs the prior in normalized [0,1] space, not the original ranges this returns), and nothing else reads dataset.prior. Looks like dead code — remove it, or if it's meant to be the analyzer's source of truth, wire it in there so the two priors can't drift apart?

[arena-review-bot]

🔵 Improvement — these tests train a full MNPE estimator on 3000 episodes (and NPE on 2000 in the other test). With no marker they run in Phase 1 (the default suite on every PR), and neural-posterior training on CPU isn't cheap. Do the planted effects still recover at a smaller num_episodes (and a capped max_num_epochs on fit)? Trimming it would keep the default phase fast without losing the ground-truth check.

[arena-review-bot]

🔵 Improvement — this (and "episode_summary.jsonl is produced by the eval runner" below) describes episode_writer / episode_summary.jsonl in the present tense, but I couldn't find either in the repo — the PR runs on synthetic data and the eval-runner integration is still "Next." Worth marking this step as planned (e.g. "will append" / a note) so readers don't go looking for a writer that isn't there yet.

[greptile-apps]

Missing version lower-bound for sbi — MNPE unavailable before v0.25.0

MNPE was introduced in sbi v0.25.0 (see changelog). With the current unpinned "sbi" spec, pip install can resolve to any older release; the first line of analyzer.py — from sbi.inference import MNPE, NPE — then raises ImportError: cannot import name 'MNPE' from 'sbi.inference' for every user who doesn't already have a recent sbi installed. Changing the spec to "sbi>=0.25.0" makes the constraint explicit and ensures the MNPE class is always present.

Suggested change

	"sbi",
	"sbi>=0.25.0",

[alexmillane]

Great work. Super clean.

I have a bunch of nits, but merge away when you see fit.

[alexmillane]

Maybe we should define what the posterior is in this case? Does the posterior mean that we have a prior integrated?

[alexmillane]

Out of interest, Do I understand correctly that the main reason to do this analysis is to capture a multi-variate distribution. Does the fact that we're modelling a posterior come into play here separately from the multivariate aspect?

[alexmillane]

jsonl is correct?

[alexmillane]

having read on, yes this is correct.

[alexmillane]

outcome (e.g. success-rate)

[alexmillane]

theta and x not introduced above. I guess this is varied parameter and the outcome?

[alexmillane]

@cvolkcvolk could be true?

[alexmillane]

We dont test any parsing of the input files (schema/dataset). Suggest to add a short test on that.

[alexmillane]

Is there a reason to store these in separate files? Does the jsonlstores the factors schema, the values of the factors taken at each experiment, and the outcomes?

It's a nit though. Happy to keep this as is. We could choose later on down the track to simplify to a single file. Let's go with this for now.

[alexmillane]

What do you think about moving this to isaaclab_arena_examples. It's more of a script demoing a feature in the core library, rather than part of the core library.

[alexmillane]

Could be worth including some little example files that are used in the analysis. I.e. the factors yaml and outcome jsonl.