Repository navigation
docs(sampling): auto-derived sampling limits + Sobol' ensemble copy + wire contract lock - #740
JavierGOrdonnez wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #740 +/- ##
===========================================
+ Coverage 88.46% 88.93% +0.47%
===========================================
Files 115 115
Lines 14703 14887 +184
Branches 1619 1638 +19
===========================================
+ Hits 13007 13240 +233
+ Misses 1609 1558 -51
- Partials 87 89 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Merging this PR will not alter performance1 warning, 1 note
Comparing 
|
There was a problem hiding this comment.
🟢 Approval recommended
The focused changes have no blocking findings; only a minor tooltip clarification remains.
1 open finding
What changed in this PR
Documents auto-derived sampling limits and aligns frontend Sobol' requests with the backend contract.
Changes:
- Explains sampling boxes and Sobol' ensemble semantics.
- Adds an in-app Sobol' information tooltip.
- Removes unused request fields and locks the wire format with a contract test.
| File | Description |
|---|---|
| SPEC.md | Records documentation and contract work. |
| README.md | Explains sampling limits and engine behavior. |
| node/src/utils/sobolIndices.ts | Removes unused request parameters. |
| node/src/utils/sobolIndices.test.ts | Locks the allowed request keys. |
| node/src/components/plots/UQPlotsSteps.tsx | Adds Sobol' explanatory copy. |
| node/src/components/plots/SobolIndicesPlot.tsx | Updates the request call. |
| node/SPEC.md | Records frontend contract and cache changes. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
a931594 to
97066c9
Compare
|
Stacking prep: branch rebased onto current |
|
/ponytail-review (over-engineering pass, diff-only)
Net diff also shrinks product code (dead domains/numSamples params out). Otherwise lean. |
…y change — regen inside pinned Playwright image on the stacked tree
…ee (chip, B59wp front, #740 values) — regen inside pinned Playwright image
97066c9 to
49bd44d
Compare
alexpargon
left a comment
alexpargon
left a comment
There was a problem hiding this comment.
Review — the wire actually goes quiet: contract verified at all three layers
Reviewed the full diff (+89/−25, 8 files) plus the backend contract on develop, the published itis-sumo==0.1.0a10 wheel (downloaded and grepped, not taken on faith), the SPEC id ledger, and the test deltas.
Good / no action needed
sobolIndices.ts+SobolIndicesPlot.tsx— the removal is true at the source, not just plausible: develop'sSobolIndicesRequest(dakota_models.py:1235) says it in its own docstring —distributions/numSamplesare "GONE from this contract",domains/fixedoptional with absent → observed-bounds auto-infer. The FE was the only side still humming the dead keys.sobolIndices.test.ts— this is the right shape of lock: sorted-key-set equality against exactly{FunctionJobs, inputLogScales, inputVars, output, outputLogScales, seed}plus an explicit banned-keys loop. A subset assertion would let the next contributor re-add a key through the side door; an equality lock can't. Existing tests updated consistently (seed pin survives, as it should — seed is a genuine wire key).- The engine claims check out at the pin. Pulled the a10 wheel from test-pypi:
api/workflows.py:205-207carries "Modeller distributions are NOT an input here; they driveevaluate_uncertainty" andevaluate/funs_evaluate.py:1288-1289carries "no box is ever re-derived from a distribution's mean ± 3σ" — the README quotes are verbatim. Andapi/_session.pysobol()really does the auto-infer: absent box →lo, hi = column.min(), column.max(), andlo == hi → fixed factorwith a comment that inventing a box around a constant "would fabricate sensitivity." Theconstant→pinnedREADME row is engine fact at this pin, not aspiration. - README two-box table — the
normal→mean±2.5σ ⇒ ≈98.8%math is right, and the framing (the ≈1.2% normal tail is outside the sampled domain = surrogate extrapolation) is the honest version of that sentence. - Ledger discipline —
V49/T51wd/B59vware all next-free (develop tops out atV48hz/T50ab/B58rs), rootT47jkfollowsT46cs,uniq -dclean on every SPEC at head. B59vw is a model cause row: tolerated extra keys →V46scderives the cache key from the sent body → guaranteed MISS + full recompute for a response that cannot depend on either. Naming the failure direction (inverse of B37rv) is exactly the register §B is for. UQPlotsSteps.tsxcopy is engine-accurate — the ensemble is uniform draws over the exploration box, log-uniform where a scale override says so (workflows.py:205), ⊥ the modeller distributions; naming Correlation as the distribution-shaped view sends users to the right panel. PNG regenerated for the new info-icon, correlation baseline left alone — the only binary in the diff isuq-readonly-sobol-indices.png(76.9→77.5 KB).
Meaningful
- Merge-order coupling with #744. #744's new
SobolIndicesPlot.test.tsxpins the as-is wire (body.numSamples= 4321,body.distributionspresent) — exactly the keys this PR deletes. The diffs don't touch a shared file, so git will merge them cheerfully; whichever lands second goes RED in node-tests until those assertions are flipped to the V49 key-set. Worth a line in either PR's merge notes so the second merger isn't debugging a phantom.
Minor (optional)
-
Mis-cite inherited, don't propagate it. The new
V49row (andT47jk's summary line) point the Saltelli fixed-count fact at../flaskapi/SPEC.md V36— but flaskapi V36 is thesanitize_varnameschar-class row. The fixed-Saltelli-base fact is flaskapi V40. The error is inherited from develop'sdakota_models.pydocstring (which cites(V36)for the same fact), but new SPEC text is the one place it's cheap to fix: pointV49at V40, and maybe weep on the backend docstring while you're there. -
Footnote on the body: "522 passed" doesn't reconcile with #744's develop baseline of 518 (+1 contract test would be 519). Raw
it(/it.each(line counts read develop 440 → here 441 (+1 ✓), so the delta is right; one of the two absolute counts is a stale re-measure. Re-run and correct the number, the delta survives either way.
Verdict: Approving on the merits — the reduction matches the backend model verbatim, the wheel claims are real at the pin, and the key-set equality test is the strongest form of lock available. Fix the V36→V40 cite when convenient (nothing depends on it), and coordinate merge order with #744 so the second landing reconciles the Sobol test instead of tripping over it.
49bd44d to
f19d086
Compare
…y change — regen inside pinned Playwright image on the stacked tree
…ee (chip, B59wp front, #740 values) — regen inside pinned Playwright image
f19d086 to
270f220
Compare
|
Rebased onto Dropped commits (already on develop). Commits 1–3 (e2e-launch-quiescence) were the stacked base branch, squash-merged via #752: the tree at Wire fix — still needed, half-done on develop. SPEC renumber (numbers squat after the Oct 21–26 merges): Develop-side test flipped: Verified: vitest 720/720, coverage 94.96% lines (≥80 ratchet), Refs JavierGOrdonnez#84 (does not close it). |
fetchSobolIndices still sent distributions/numSamples although the backend SobolIndicesRequest dropped both with the bounds-editor removal: pydantic tolerates the extras, but V46sc keys the session cache off the SENT body, so every distribution/numSamples edit was a guaranteed cache miss + full recompute of a response that cannot depend on them (node B59vw). The Sobol' box is the engine's auto-inferred DOMAIN (observed bounds, constant column -> fixed; root JavierGOrdonnez#84) - domains/fixed must also never appear. Full key-set contract test locks node V49.
…le copy Fork #84 (docs + copy PR): README gains 'Sampling limits (auto-derived)' — LHS/grid box seeded uniform->/[min,max], normal->mean +/- 2.5 sigma (~98.8% mass, the ~1.2% normal tail sits outside the sampled domain = extrapolation), constant->pinned; the Sobol' box is auto-inferred backend-side in itis-sumo from observed training bounds (uniform draws, count fixed in the engine); the two boxes differ (cross-ref itis-sumo#55). Engine-pin caveat closed by inspecting the published itis-sumo==0.1.0a10 wheel: the auto-infer + both docstring quotes #84 cites from a11 are already in a10. The UQ Sobol' step now carries header info copy stating the indices describe the surrogate's uniform ensemble over its domain box, not the user's distributions (the Correlation panel is the distribution-shaped view). Sobol baseline regenerated IN the pinned Playwright image (info icon lands in the header; verify-only pass green against the shipped baseline set, V10/V12). SPEC: root T47jk, node V49/T51wd/B59vw.
270f220 to
419b1be
Compare
|
Review-round status — all Copilot and @alexpargon items addressed on
|
…y change — regen inside pinned Playwright image on the stacked tree
…ee (chip, B62wp front, #740 values) — regen inside pinned Playwright image
…y change — regen inside pinned Playwright image on the stacked tree Rebase round 2 (new base @ develop): three more baselines had aged past the base itself and were regenerated + cross-boot-verified inside the pinned image: - moga-readonly-pareto.png / moga-readonly-settings-modal.png: develop #633 added the Performance column to the Pareto table (settings-modal was masked behind the pareto assertion in CI run 1). - sobol-highdim-second-order-heatmap.png: the base's T47mx container shot (mmux-testid="sobol-indices-plot") now includes the sweep's download chip seated in that plot container — the established V36ez chip-in-baseline precedent.
…ee (chip, B62wp front, #740 values) — regen inside pinned Playwright image Base-move refresh (onto re-based #741 @ 4384982): the three MOGA read-only baselines re- aged again — the base moved in develop's Pareto-table Performance column (#633) on top of this tree's full-front envelope render. Re-gen disciplined per LANE context: monolithic update-mode runs leak shared-backend state (a continued-past-failure spec mutates the job collections the SuMo/UQ views read), so only the MOGA lane's own files were taken from the pinned-image regen; SuMo/UQ baselines stay at the lane-verified bytes the base ships.
…main (B63px) The column was the terminal frame of three async writes in arbitrary order: - the table's [weights] effect overwrote row.performance with a naive direction-blind raw sum (empty e2e-seeded weights -> 0.00) - the parent's [weights] recalc wrote the normalized score into a capital-Performance key the grid never reads (dead write) - runMOGA's own commit normalized over ALL evaluations while the recalc re-derived min/max from the front rows; a single-objective front is all rows tied at the optimum -> min==max -> norm=0 fallback -> whole column 0.0, and the runMOGA closure could hold empty weights -> all cells NaN (the '-' placeholder) Pixels settled on '-', 0.00, 0.0 or the resolved 1.0 by write order (proven: pinned-image regens of one commit -> resolved/-/resolved; post single-writer verifies -> 0.0/0.0/1.0/1.0). The parent recalc is now a pure projection of (tableData, weights, outputTargets): it fires on every tableData commit and every weights/direction edit, writes only the lowercase rendered field, always normalizes against raw (the same all-evaluations domain runMOGA uses), and bailouts via functional update when every row already agrees, so terminal pixels are write-order independent instead of a coin flip. - regression test MOGAParetoTable.test.tsx: given scores render verbatim (red on the old child code, green on the fix) - e2e: expectMogaPerformanceComputed gate (cells exist, then no '-' placeholder) before every full-page MOGA baseline guards the transient window; baselines regenerate in the pinned image in a follow-up commit Refs JavierGOrdonnez#83, node V49wp/B59wp/T51ic Re-base adaptations (base moved to develop d764875 + re-based #740-#742): - the projection effect now falls back to {} for a function with no outputTargets entry (same idiom as the scale-detection guard) — the develop #678 lifecycle test "falls back to an empty objective set" caught the un-guarded Object.keys crash. - #633's parent-matrix test read the DEAD capital-Performance key the convergence deletes: reads re-targeted to the lowercase rendered field (values identical — the raw all-evaluations domain contains the front rows). - #633's table test pinned the retired child recomputation: re-titled to assert the GIVEN scores stand, no realign fires on a weights-only edit (realign is keyed to [data] now) and the raw-sum override never appears.
…main (B63px) The column was the terminal frame of three async writes in arbitrary order: - the table's [weights] effect overwrote row.performance with a naive direction-blind raw sum (empty e2e-seeded weights -> 0.00) - the parent's [weights] recalc wrote the normalized score into a capital-Performance key the grid never reads (dead write) - runMOGA's own commit normalized over ALL evaluations while the recalc re-derived min/max from the front rows; a single-objective front is all rows tied at the optimum -> min==max -> norm=0 fallback -> whole column 0.0, and the runMOGA closure could hold empty weights -> all cells NaN (the '-' placeholder) Pixels settled on '-', 0.00, 0.0 or the resolved 1.0 by write order (proven: pinned-image regens of one commit -> resolved/-/resolved; post single-writer verifies -> 0.0/0.0/1.0/1.0). The parent recalc is now a pure projection of (tableData, weights, outputTargets): it fires on every tableData commit and every weights/direction edit, writes only the lowercase rendered field, always normalizes against raw (the same all-evaluations domain runMOGA uses), and bailouts via functional update when every row already agrees, so terminal pixels are write-order independent instead of a coin flip. - regression test MOGAParetoTable.test.tsx: given scores render verbatim (red on the old child code, green on the fix) - e2e: expectMogaPerformanceComputed gate (cells exist, then no '-' placeholder) before every full-page MOGA baseline guards the transient window; baselines regenerate in the pinned image in a follow-up commit Refs JavierGOrdonnez#83, node V49wp/B59wp/T51ic Re-base adaptations (base moved to develop d764875 + re-based #740-#742): - the projection effect now falls back to {} for a function with no outputTargets entry (same idiom as the scale-detection guard) — the develop #678 lifecycle test "falls back to an empty objective set" caught the un-guarded Object.keys crash. - #633's parent-matrix test read the DEAD capital-Performance key the convergence deletes: reads re-targeted to the lowercase rendered field (values identical — the raw all-evaluations domain contains the front rows). - #633's table test pinned the retired child recomputation: re-titled to assert the GIVEN scores stand, no realign fires on a weights-only edit (realign is keyed to [data] now) and the raw-sum override never appears.
| // ROOT #84: the Sobol' box is auto-derived engine-side and ⊥ editable — the | ||
| // copy must name whose ensemble the indices describe (README "Sampling limits"). | ||
| "Sobol' Indices": | ||
| "Sobol' indices describe the surrogate's own exploration ensemble: itis-sumo draws uniformly over the surrogate's domain box (log-uniform for variables flagged log-scale, matching the README \"Sampling limits\" contract), auto-derived from the observed training-sample bounds (a constant column stays fixed). These sampling limits are not editable. To see your input distributions pushed through the surrogate, use the Correlation panel.", |
There was a problem hiding this comment.
this reads verbose; pls use the humanizer skill.
Remove the comment lines as well.
This could be something like:
Sobol: "Sobol' indices describe the importance of each parameter for the output QoI over the full surrogate domain, infered from training samples."
What
Implements the remaining acceptance criteria of JavierGOrdonnez/mmux_vite#84 (parent: #63 DOMAIN ⊥ DISTRIBUTION): the communication + wire-lock half. FE domain only (README +
node/;flaskapi/+ engine untouched). Rebased ontodevelop@d764875b; the former stacked base (e2e-launch-quiescence commits) was dropped — it is byte-identical to the #752 squash already on develop.1. Docs — README "Sampling limits (auto-derived)"
uniform → [min, max],normal → mean ± 2.5σ(≈98.8% mass; the ≈1.2% normal tail lies OUTSIDE the sampled/validated domain = surrogate extrapolation),constant → pinned.itis-sumo(observed sample bounds; constant column → fixed factor; uniform draws, log-uniform on the log flag; Saltelli count fixed inside the engine) — no editable UI; honest note that the LHS/grid forms seed from the distributions and stay adjustable before launch.mean ± 2.5σ) documented as different boxes, reconciliation tracked in itis-sumo#55. Nodocs/dir created — that decision belongs to Increase version number #61.2. In-app copy (node)
The UQ Sobol' step carries the header info tooltip (
UQPlotsSteps): indices = the surrogate's uniform ensemble over its auto-derived domain box (log-uniform where flagged — Copilot review), limits not editable, Correlation named as the distribution-shaped view.3. Contract test (wire lock, node V50)
fetchSobolIndicesbody reduced to the live contract keys{inputVars, output, FunctionJobs, seed, inputLogScales, outputLogScales}; the new key-set test locks the FULL set sodomains/fixed/distributions/numSamplescan never regrow onto the wire. Still needed after the Oct-26 refactor:54a179echad already droppednumSamplesbut leftdistributionssending, whileSobolIndicesRequestdocuments both as GONE from the contract — so theV46sccache-key pollution (node B60vw) is live on develop until this lands.SobolIndicesPlot.test.tsx's wire-shape pin (from #744/#633) asserteddistributionsON the wire — inverted to the negative in the fix commit, resolving the merge-order coupling alexpargon flagged (their "Meaningful" item is now moot: #744 already landed).4. Engine-pin caveat verified on the pin itself
pyprojectpinsitis-sumo==0.1.0a10while #84 quotesa11docstrings. Inspected the published a10 wheel: auto-infer implemented (api/_session.py: optionaldomains/fixed, observedmin/maxinference, constant column → fixed factor, uniform/log-uniform draws) and both docstring quotes present in a10 ⇒ the contract holds for the shipped pin. (alexpargon independently re-verified against the wheel.)Review round 2 (this push)
flaskapi/SPEC.mdV40, not V36 — cites corrected in node V50 (develop-side copies of the mis-cite, incl. thedakota_modelsdocstring, are outside this PR's §C FE domain).not.toHavePropertyloop deleted (the exact-settoEqualalready REDs on any added key); duplicated description comments thinned acrosssobolIndices.ts/.test.ts,UQPlotsSteps.tsx, README (−20 lines net; ledger rows untouched — the SPEC §B/§T rows are the canonical home of the narrative).Verification
tsc -b,knip, eslint (0 errors),uvx prekgreen.uq-readonly-sobol-indices.pngregenerated inside the pinnedplaywright:v1.64.0-nobleimage (info icon lands in the step header, V10/V12) and passes there.Commits
fix(sobol): wire body = live contract keys only + V50 key-set contract test (incl. inverting test(node): adopt 80/80/80/80 coverage ratchet (V27ku) via focused branch-gap sweep (#633) #744's stale wire-shape pin)docs(sampling): README section + Sobol' panel copy + SPEC rows + regenerated baselinedocs(node): Sobol tooltip carries the log-uniform qualification (Copilot review)Refs JavierGOrdonnez/mmux_vite#84 (does not close it — recommend checking its acceptance boxes on merge).