Repository navigation
Builder APIs disagree on setter naming, entry, and terminal method #434
Description
Problem
PECOS has fifteen Python-facing builders and they disagree on all three parts of the
builder contract: how setters are named, how the builder is entered, and how it is
terminated. The split is close to even, so a reader cannot infer any of it.
Setter naming: 8 versus 7
with_* |
bare |
|---|---|
general_noise() (47 methods) |
sim(...) |
depolarizing_noise() (7) |
DetectorErrorModel.builder() (14) |
biased_depolarizing_noise() (7) |
BpOsdBuilder (5) |
DemBuilder (6) |
BpLsdBuilder (4) |
DemSamplerBuilder (4) |
RelayBpBuilder (8) |
InfluenceBuilder (4) |
MinSumBpBuilder (3) |
SurfacePatchBuilder (3) |
UnionFindBuilder (1) |
ColorCode488Builder (1) |
Two plausible dividing lines both fail:
- user-facing versus low-level — the noise builders are user-facing and use
with_*,
whilesim()is user-facing and bare. - Rust-backed versus Python —
BpOsdBuilderis Rust-backed and bare,DemBuilderis
Rust-backed andwith_*.
Terminal method
.build()— most builders.run()—sim(...)- none — the noise builders, which are passed directly into
.noise(...)
Entry
- free function:
sim(...),general_noise(),depolarizing_noise() - classmethod:
DetectorErrorModel.builder() - direct construction with required positional arguments:
BpOsdBuilder(pcm, error_rate)
Why now
PR #420 added DetectorErrorModel.builder() and chose bare setters partly on the
strength of a "user-facing builders use bare setters" reading of the codebase. That
rule does not hold — see the counterexamples above. The choice is defensible on other
grounds (it is scalar configuration on a Python surface, and the Rust API Guidelines
leave builder naming explicitly open, reserving with_ for constructors such as
Vec::with_capacity), but it was made against a pattern that turned out to be imaginary.
Rather than let that stand as an accidental precedent, the project should decide.
Proposal
- Pick one setter convention and write it down where it will be found — a short
section in the contributing or API-conventions docs, not just an issue comment. - Standardize the terminal.
.build()everywhere, withsim(...).run()as a
deliberate, documented exception because it also executes. - Decide whether noise builders should have a terminal at all, or remain
builder-shaped objects consumed directly by.noise(...). - Converge entries on one shape, or state the rule that selects between them.
- Migrate with aliases and a deprecation ramp, as in Deprecation ramp: align pecos-engines GeneralNoiseModel idle-noise API with the structured vocabulary #424 — several of these are public
and widely used.
Notes
- Any convention chosen should be applied to the new
DetectorErrorModel.builder()too;
it is new enough to change cheaply and has no external callers yet. - Related: Reshape the Guppy DEM construction API into the house builder pattern #428 (which introduced the builder), Rename the DEM-construction NoiseModel to say it is a parameter set, not a simulator noise model #433 (
NoiseModelnaming), Deprecation ramp: align pecos-engines GeneralNoiseModel idle-noise API with the structured vocabulary #424/Align the non-idle GeneralNoiseModel surfaces (gate/emission/leakage/measurement/crosstalk) with the structured noise vocabulary #425
(the engines-side vocabulary ramp, where a deprecation pattern is already planned).
Activity
ciaranra commentedon Aug 4, 2026
A dividing line that fits the data, arising from the #433 decision (
NoiseParametersadoptingwith_*to match thesim()noise builders):with_*and have no terminal. They describe a thing and are handed to something else:general_noise(),depolarizing_noise(),biased_depolarizing_noise(), andNoiseParametersonce renamed. All are passed into.noise(...).sim(...).run(),DetectorErrorModel.builder().build().This is a real distinction rather than a post-hoc rationalization — the two groups differ in what they return and how they are consumed, not just in age or implementation language.
It does not cover everything, and the leftovers should be decided explicitly rather than quietly:
DemBuilder,DemSamplerBuilder,InfluenceBuilderare pipeline builders with terminals but usewith_*. Under this rule they would migrate to bare — but they are the low-level/advanced surfaces (DemBuilder's own docstring points users tofrom_circuit()), so grandfathering them is defensible if stated.SurfacePatchBuilderandColorCode488Builderconstruct code objects; they fit "orchestration" better than "configuration".BpOsdBuilderand siblings) are already bare with.build(), so they satisfy the rule as-is.If this line is adopted, the
DetectorErrorModel.builder()added in #428 is already correct, andNoiseParametersmoves towith_*under #433.ciaranra commentedon Aug 4, 2026
The parameter-object half of this is settled by a hard constraint, not taste.
NoiseParametersis a dataclass whose fields are public attributes (noise.p1,noise.p_meas,noise.p_idle_linear). A bare setter namedp1()would shadow the field of the same name, so bare setters are not available for a parameter object at all —with_<field>is forced.That also explains why
general_noise()had freedom the parameter object does not: it is a builder with no public fields of those names.So the target convention is:
with_<field_name>, no terminal. Forced by the attribute collision.NoiseParametersnow follows this.sim(...).run(),DetectorErrorModel.builder().build().general_noise(),depolarizing_noise(), andbiased_depolarizing_noise()already use thewith_prefix, so they are aligned on that axis. The remaining divergence is the suffix: they spell itwith_p1_probability/with_p_idle_linear_ratewhereNoiseParametersuseswith_p1/with_p_idle_linear.Convergence work, for #425:
with_p1,with_p2,with_meas/with_p_meas,with_prep/with_p_prep) to the simulator noise builders, matching the field-name rule.with_p_idle_linear_ratecorresponds to what is our deprecated Z-only aliasp_idle_linear_rate, not to the family scalarp_idle_linear. Renaming without resolving that would create a false equivalence between two different quantities.Item 3 is the reason this cannot be a mechanical find-and-replace.