refactor(experiments): make the trade-off API names consistent - #491
Merged
Conversation
The four trade-off functions had drifted into four separate naming problems: the same word was spelled two ways in sibling functions (tradeoff against trade_off_table); `runs` was an int in one function and a tuple in another, with no cue at the call site; the OMARS pair changed parameter names between its own two functions; and trade_off_table was the only one of the four without a `display` argument. On top of that, `omars_tradeoff` was both a module and a function in it. Three rules now govern the group, and they are meant to apply to any future pair: one spelling, `trade_off`; the entry accessor is named get_<table_name>_entry; and scalar counts take the n_ prefix while sequences stay plural bare nouns, so the parameter type is visible at the call site. tradeoff(runs, factors) -> get_trade_off_table_entry(n_runs, n_factors) omars_tradeoff(n_runs, n_factors) -> get_omars_trade_off_table_entry(n_runs, n_factors) TradeoffResult -> TradeOffTableEntry OmarsTradeoffResult -> OmarsTradeOffTableEntry experiments.tradeoff -> experiments.trade_off experiments.omars_tradeoff -> experiments.omars_trade_off TradeOffTableEntry.runs and .factors become .n_runs and .n_factors, matching OmarsTradeOffTableEntry, which already had them. trade_off_table gains `display`, defaulting to True like the other three. No deprecation shims: the old names are gone. The trade_off_table MCP tool is deliberately untouched. Same tool name, same runs and factors schema keys, same output; only its internal call site moved, so no MCP client breaks. A scan of the public API (99 modules, 287 public functions) confirms no compound word is now spelled two ways anywhere. It also surfaced scalar counts without the n_ prefix in generate_design (`replicates`, `blocks`) and its helpers; those are left alone pending a decision, since generate_design is itself an MCP tool. Version bump deferred: an incompatible rename without deprecation is semantically MAJOR, but the deprecation policy permits MAJOR only after its announce and warn schedule has run. The changelog entry sits under [Unreleased] until that is settled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W1daHPaXLs7PPRsq8CmMMm
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…ay argument Codecov flagged one partial branch on the PR: every existing test called trade_off_table() with display left at its default, so the quiet path added in the previous commit was never taken. Adds a test that asserts both directions, and switches the three existing table tests to display=False so the suite stops printing the whole table to stdout now that True is the default. trade_off.py is back to 100% branch coverage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W1daHPaXLs7PPRsq8CmMMm
…ters Completes the naming sweep started with the trade-off functions. The rule is that a scalar count takes the n_ prefix while a sequence keeps a plural bare noun, so the parameter's type is visible at the call site. center_points -> n_center_points replicates -> n_replicates blocks -> n_blocks (the COUNT only) Touches generate_design, build_design_result, replicate_design, dispatch_ccd, dispatch_box_behnken, allocate_budget, estimate_rsm_runs, and TradeOffTableEntry.n_replicates. Deliberately NOT renamed: DesignResult.blocks stays as it is. It holds per-run block assignments as a list[int], and sequences keep the plural bare noun. Renaming the count actually removes a collision here, because build_design_result took a blocks count and returned a blocks list from the same function. blocks throughout the multivariate package means a sequence of data blocks in multiblock PCA and PLS. Different concept, already correct, left alone. generate_omars(n_runs_range) is n_-prefixed and tuple-valued, but it reads as "the range for n_runs" rather than a sequence of counts, so it stays. Two things worth calling out. The generate_design MCP tool schema changes, since center_points and replicates are input keys. And the per-key DoS caps in tool_safety.py had to be re-keyed to match: they are keyed by tool input name, so without that change the caps for these two inputs would have stopped applying silently. tests/test_sec19_dos_caps.py caught it. A re-run of the API scan now reports no scalar count anywhere in the library missing the n_ prefix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W1daHPaXLs7PPRsq8CmMMm
…epair prose
Two things, both in the DOE code.
1. analyze_experiment now reports estimability.
A rank-deficient model matrix is still "fitted" by the pseudo-inverse: a
coefficient is returned for every requested term, with a standard error and a
p-value, while only `model_rank` of them are determined by the data. The rest are
one arbitrary solution out of infinitely many. Economical designs that carry
structured aliasing (definitive screening, OMARS, foldovers generally) land in
that state routinely, so the caller was being handed confident-looking output
with nothing to flag it.
model_summary gains n_terms, model_rank and rank_deficient, and a RuntimeWarning
fires when the model is deficient, naming how many terms are not estimable and
saying what is and is not affected: predictions at the design points are fine,
individual coefficients and predictions elsewhere are not.
Demonstrated on the JMP definitive-screening example (6 factors, 17 runs): the
full second-order model asks for 28 terms and gets rank 15, which previously came
back silently. The main-effects-plus-quadratics model on the same data is full
rank and stays silent.
2. Prose damaged by the parameter rename.
The n_ prefix sweep in the previous commit was applied by pattern, and it reached
23 places where "replicates", "blocks" or "center points" was the English word
rather than an identifier: docstring descriptions, comments, an error message
("Number of n_blocks must be at least 2."), a strategy purpose string ("Run
n_replicates at the predicted optimum"), and two MCP schema descriptions visible
to tool callers ("Number of center point n_replicates"). All restored.
The lesson is recorded rather than just fixed: a rename by pattern needs a
separate prose pass, because tests and type checks pass either way. Nothing here
was caught by ruff, mypy or pytest; it was found by reading the diff.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W1daHPaXLs7PPRsq8CmMMm
Codecov flagged one uncovered line on the PR: the fractional-factorial branch of _screening_design_params. The rename touched that line, so it surfaced as a patch gap, but the function had no direct test at all: three of its four branches were only ever reached incidentally, and one never. Adds a test per branch, including that definitive screening asks for a fake factor only when the factor count is even. engine.py goes from 94 to 95 percent. The remaining uncovered lines in that module predate this branch and are left alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W1daHPaXLs7PPRsq8CmMMm
#485 landed the doe-designer skill and minimum moment aberration. Merging it in exposes call sites that this branch's renames break, none of which git flags. CHANGELOG was the only textual conflict: both sides had added to [Unreleased]. Entries from both are kept, main's first in each subsection. Three schema-key call sites in the skill. The n_ prefix sweep renamed generate_design's `center_points` and `replicates` inputs, and the skill passes those keys as live JSON: examples/design_spec.json "center_points": 0, invoked by SKILL.md, examples/README.md and worked-example.md references/worked-example.md:59 the prose describing that call references/worked-example.md:199 a runnable CCD command Left alone, the skill's flagship example would fail input validation rather than degrade. All three now use n_center_points, and all three documented commands were run to confirm it. The English words are untouched: "center points or replicates", "extra replicates", "3 replicates at the optimum". Only keys in payloads changed. Three tests in test_moment_aberration.py called generate_design with center_points=; updated to n_center_points=. Also corrected, unrelated to the rename: the CHANGELOG entry for the skill said it exposes "the ten designed-experiments tools". trade_off_table (#487) made it eleven, which SKILL.md already reflected after the #485 merge. Citation narrowed in three places. SKILL.md, references/verification.md and scripts/verify_design.py each stated that the Vazquez, Rother and Charles-Gonzalez study (arXiv:2512.17113) ran GPT-5.1 and Gemini 2.5 Flash over 36 tasks with ten replicates, and that the failures were resolution 1 or 2, non-regular arrays offered as regular fractions, and tables with missing cells. The paper's abstract supports the run sizes, the factor range, the GPT and Gemini families, and reliability up to about eight factors; it does not name model versions, task or replicate counts, or those specific failure modes. The text now claims only what is verifiable, and the authors are named in full. Verification after the merge: ruff check . and ruff format --check . clean mypy src/process_improve clean, 146 source files pytest: 2538 passed, 5 skipped API naming scan over the merged code: no scalar count missing the n_ prefix, including everything #485 added all 44 numeric checks behind the pid-book sections still pass MCP surface as intended: trade_off_table still keyed on runs and factors, generate_design now on n_center_points and n_replicates skill scripts run end to end, including verify_design.py --require-resolution 5 exiting 3, and the CCD command from worked-example.md Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W1daHPaXLs7PPRsq8CmMMm
…inventory Three accuracy problems found in a final sweep of the documentation. user_guide/omars_designs.rst said the frontier is strictly more than the parameter count "from four factors up". It is more at every factor count: the gap is exactly k(k-1)/2, so it is 3 runs at three factors, not zero. The phrase described where the old sizing bug showed up, not where the gap exists. The text now states the gap and says the parameter count is never a sufficient test. doe/tools.md documented generate_design's schema with center_points, replicates and blocks, which the n_ prefix rename has since changed. These are the input keys an agent sends, so a stale table here is a wrong contract. doe/README.md and doe/tools.md both claimed eight tools, with two not started. There are eleven, all implemented. The coverage percentages in that table were measured against the 162-question bank for the original eight; the three added since have not been scored, and the text now says so rather than implying the figures cover them. Also checked and correct as they stand: the performance table's half-pool column against (3^k - 1)/2, its run-size and error-df columns against k^2+k+1 and the parameter count, and the solver timings against the prose. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W1daHPaXLs7PPRsq8CmMMm
The tool count appeared in five places and had to be bumped every time a tool was added, which is how it came to read 'eight tools, two not started' long after there were eleven and all of them worked. The count is also not the useful fact: what matters is what each tool does. Removed from doc/doe/README.md (both the At a Glance line and the '8-tool architecture' file description), docs/doe/tools.md, SKILL.md, the CHANGELOG entry for this branch, and the simulation tools module docstring. Where a reader needs to know what exists, the text now points at the registry instead: get_tool_specs() in the library, and doe_tool.py list in the skill. Both enumerate whatever is actually registered, so they cannot drift. The summary table in docs/doe/tools.md is completed with create_factorial_design, fit_linear_model and trade_off_table. Their coverage columns carry a dash: the question-bank percentages were measured before those tools existed, and scoring them retrospectively would mean inventing numbers. The table now matches the registry exactly, checked programmatically. Historical CHANGELOG entries that state a count are left alone. They describe what a past release did and are correct as a record. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W1daHPaXLs7PPRsq8CmMMm
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The four trade-off functions had drifted into four separate naming problems:
tradeoffagainsttrade_off_table.runswas anintintradeoff()and atupleintrade_off_table(). Same parameter name, different type, no cue at the call site.n_runs/n_factorsagainstruns/factors).trade_off_tablewas the only one of the four without adisplayargument.On top of that,
omars_tradeoffwas both a module and a function inside it.Rules this establishes
Meant to apply to any future pair, not just this one:
trade_off, nevertradeoff.get_<table_name>_entry.n_prefix; sequences stay plural bare nouns. The parameter type is then visible at the call site.display.What changed
tradeoff(runs, factors)get_trade_off_table_entry(n_runs, n_factors)omars_tradeoff(n_runs, n_factors)get_omars_trade_off_table_entry(n_runs, n_factors)TradeoffResultTradeOffTableEntryOmarsTradeoffResultOmarsTradeOffTableEntryexperiments.tradeoff(module)experiments.trade_offexperiments.omars_tradeoff(module)experiments.omars_trade_offTradeOffTableEntry.runsand.factorsbecome.n_runsand.n_factors, matchingOmarsTradeOffTableEntry, which already had them.trade_off_tablegainsdisplay, defaulting toTruelike the other three, so a baretrade_off_table()now prints as well as returns.Renaming the modules also removes the module/function collision.
No deprecation shims. The old names are gone.
MCP surface: deliberately untouched
Same tool name (
trade_off_table), samerunsandfactorsschema keys, same output. Only the internal call site moved, so no MCP client breaks. Verified by calling the tool after the rename.Consistency scan
A scan of the public API (99 modules, 287 public functions) confirms no compound word is now spelled two ways anywhere in the library. It also surfaced two things left alone pending a decision, since
generate_designis itself an MCP tool and renaming its parameters would be a schema change:generate_design(replicates: int)andgenerate_design(blocks: int | None): scalar counts without then_prefix.replicatesalso appears indesigns_utils.build_design_resultandreplicate_design.center_pointsis in the same category and was not flagged by the scan; consistency would pull it in too.generate_omars(n_runs_range: tuple[int, int])was flagged asn_-prefixed but sequence-valued. It reads as "the range forn_runs", so I judged it fine.Verification
ruff check .cleanruff format --check .cleanmypy src/process_improveclean, 145 source filespytest: 2475 passed, 5 skipped, coverage 94.08%trade_off_tableMCP tool called after the rename: unchanged schema and outputOpen question: version number
Not bumped yet, and the changelog entry sits under
[Unreleased].An incompatible rename without deprecation is semantically 2.0.0. But
docs/development/deprecation_policy.rstpermits MAJOR only after its announce and warn schedule has run, which was deliberately skipped here. So the options are 2.0.0 (correct semantics, contradicts the written policy) or 1.67.0 (understates a breaking change).CLAUDE.mdsays to ask rather than pick when unsure, so this is left for the maintainer.Downstream
pid-bookandkgdunn/figuresboth call the renamed functions and need matching updates before this merges:pid-book:design-analysis-experiments/omars-designs.rst,design-analysis-experiments/fractional-factorial-designs/design-resolution.rstfigures:doe/omars-capability-staircase.pyGenerated by Claude Code