A cost model reaches an API, not only an ambient setting - #1102
A cost model reaches an API, not only an ambient setting#1102Rafael-SOWNet wants to merge 1 commit into
Conversation
#746 tier 2's own row names this remaining, in its own words: "a cost model that reaches an API rather than an ambient setting". Checked against the actual API surface rather than assumed: `Entity.Simplify(int level = 2)` has no overload taking a `CostModel`, and the only way to change what candidate `Simplify` prefers is `MathS.Settings.ComplexityCriteria.Set(...)` around the call. That setting was already safe to use -- `Setting<T>` is async-local, not thread-static, so one caller's override cannot leak into another's concurrent call, the same design `MathS.Settings.Budget` and `BudgetLedger.For` already share -- but it is not a place in the addressable API `Transformation` is, which is what tier 2's own wording asks for. `Transformation.SimplificationAtLevel(int level, CostModel costModel)` is that place. It does not thread an explicit cost parameter through `Simplificator` -- that would be a second candidate-search pipeline to keep in step with the one `Entity.Simplify(int)` actually runs, which is exactly the risk the existing pipeline was written once to avoid. It scopes the existing, already-tested `ComplexityCriteria` setting for the duration of this one call instead, so `Simplify`'s own behaviour is the thing being configured rather than a parallel implementation of it. `CostModel.Default` given explicitly behaves identically to the overload without one, and passing it is how a caller states that on purpose. TDD: a spy `CostModel` proves the given model is the one actually consulted (not the ambient default); a before/after check on `MathS.Settings.ComplexityCriteria.IsOverriden` proves the scope does not leak past the call; and `CostModel.Default` given explicitly is checked against the plain overload to prove it changes nothing when the caller asks for what was already happening. `PublicApi.txt` regenerated for the one new public member. Full suite: 8797 tests, 8783 passed, 14 skipped (pre-existing), 0 failed.
|
A code review flagged one finding against this PR specifically: Recorded rather than fixed here, alongside thirteen other findings from the same review pass (mostly against #1101): |
1 participant
Summary
#746 tier 2's own row names this remaining, in its own words: "a cost model that reaches an API rather than an ambient setting." Verified against the actual surface rather than assumed —
Entity.Simplify(int level = 2)has noCostModeloverload, and the only existing way to change whatSimplifyprefers isMathS.Settings.ComplexityCriteria.Set(...)around the call.Transformation.SimplificationAtLevel(int level, CostModel costModel)— the new, addressable place. It does not thread an explicit cost parameter throughSimplificator(a second candidate-search pipeline to keep in step with the oneSimplifyactually runs); it scopes the existing, already-testedMathS.Settings.ComplexityCriteriasetting for the duration of one call.CostModel.Defaultgiven explicitly behaves identically to the overload without one — passing it is how a caller states that on purpose.Independent of #1101 — this doesn't touch the e-graph work at all, so it's cut from
masterdirectly rather than stacked.Test plan
CostModelproves the given model is the one actually consulted, not the ambient default.MathS.Settings.ComplexityCriteria.IsOverridenis checked false before and after the call, proving the scope doesn't leak.CostModel.Defaultgiven explicitly is checked against the plainSimplificationAtLevel(level)overload — identical output.PublicApi.txtregenerated for the one new public member — diff is exactly that one line.🤖 Generated with Claude Code