Encoder optimization, remove at_position, axis-aware explanations#12
Merged
antonstefer merged 28 commits intomainfrom Apr 15, 2026
Merged
Encoder optimization, remove at_position, axis-aware explanations#12antonstefer merged 28 commits intomainfrom
antonstefer merged 28 commits intomainfrom
Conversation
Values were unconditionally lowercased in label() and objectValue(), breaking proper nouns like ship names and place names. Flip the default: values now preserve casing unless the category opts in with lowercase: true. Add the flag to all default categories with adjective/common-noun values (Color, Pet, Drink, Food, Hobby, Music, Sport).
Replace hardcoded "position"/"in" in all deduction explanation strings with the first ordered category's noun and values. Explanations now say "the first house" instead of "the first position" for House-axis puzzles, and use the axis noun for any ordered category (e.g. "rank").
Replace the O(M³·n³) rank-forbidding between encoder and O(M²·n²) binary encoder with rank auxiliary variables. Define r(v,k) = "value v has rank k on axis", linked via forward channeling + AMO + ALO. Between constraints now emit 3-literal clauses over rank vars instead of 6-literal clauses over position×rank pairs. Binary constraints use 2-literal rank clauses instead of 4-literal. Clause count drops from ~260k to ~1k at n=8. The RankVarAllocator caches per (axis, value) pair to ensure channeling clauses are emitted exactly once across all constraints on the same axis.
Remove the two-path encoder asymmetry: all comparative constraints now use the rank-var encoder regardless of axis. Delete 7 positional fast-path encoder functions and the identity-pinned deduction branches. Replace at_position/not_at_position constraint types with same_position/not_same_position against display axis values. Remove the "direct" and "elimination" deduction techniques. The display axis is still pinned for symmetry breaking, but all other categories use the general rank-var path uniformly. BREAKING: atPosition() and notAtPosition() exports removed.
When the constraint's axis is the pinned display axis, rank = position by construction. Use position variables directly instead of allocating rank auxiliary variables and channeling clauses. Saves ~(V·M·n + V·M²) clauses per pinned-axis constraint.
- Cache AxisTerms on DeduceState instead of recomputing per step - Extract lc() helper in templates.ts for the repeated lowercase conditional - Make alloc parameter required on encodeConstraint (was optional but always needed) - Fix SILENT_STEP using removed "direct" technique type - Single-pass between rank validation instead of three passes - Fix stale at_position reference in verb JSDoc
Remove dead dedup() function and its calls — the callers never produce duplicate eliminations. Remove unreachable ordinal() fallback in computeAxisTerms. Add tests for contradictory and duplicate unit clauses in SAT solver, and for ordered categories with no noun. Restructure processUnitClauses for v8 coverage accuracy.
When the constraint's axis is pinned (rank = position), skip axisRankDomain/projectRanksToPositions and work directly with possible position sets. Recovers ~25-30% of the deduction regression at 6x6/8x8 grid sizes.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
logic-grid | 4553723 | Commit Preview URL Branch Preview URL |
Apr 15 2026, 02:38 PM |
- theme.ts: rewrite positionAdjective docs to describe same-position inversion instead of removed at_position - rewrite.test.ts: drop duplicate same_position assertion - Rename tryBinaryRankSpace → tryBinaryAxis and tryBetweenRankSpace → tryBetweenAxis since they handle both pinned (position) and non- pinned (rank) axes
Add because-context to tryBinaryAxis (next_to, not_next_to, left_of, before, exact_distance) and tryBetweenAxis (between, not_between). Between joins all non-empty anchors since its deductions usually depend on multiple values' positions. Capture state via describeKnown BEFORE mutating possibilities so the context reports the TRIGGER, not the conclusion. Same fix applied to trySamePosition which had the same pre/post-mutation bug. Before: "Cat is between Alice and Blue on House. Cat can't be in the first or fourth house." After: "Cat is between Alice and Blue on House. Alice is in the fourth house and Blue is in the second house, so Cat must be in the third house."
The size <= 3 cutoff was fine before because callers sampled AFTER mutation, so fresh full-range possibles never triggered. With the pre-mutation capture for because-context, full-range values emit "X can only be in the 1st or 2nd or 3rd house" on a 3-grid — true but useless, and shadows informative pinned descriptions in trySamePosition (knownA wins over knownB even when knownA is vacuous). Add < state.n guard so we only emit when the domain is actually narrower than the full range.
(1) Display-axis operand direct phrasing: when same_position or not_same_position involves a display-axis value (e.g. "first"), use "X must be in the <axisVal> <noun>" / "X is not in the <axisVal> <noun>" instead of the symmetric/tautological form. describeKnown also skips the tautology for pinned axis values to avoid shadowing informative operands. (2) Restore old pinned-axis wording in comparative descriptions: "directly left of", "somewhere left of", "next to", "not next to" instead of the generic semantic names. Drop " on <Axis>" suffix for single-axis grids (retained for multi-axis disambiguation). (3) Document varCeiling's capture-after-allocation contract. (4) Replace "axisName:value" string cache key with nested Map<Category, Map<string, number>> — collision-safe. (5) Document SILENT_STEP.technique as an opaque placeholder.
Replace the bad-pairs encoder (|bad pairs| × 1 binary clauses) with implication form: for each rank i of a, emit [¬a@i, b@j₁, b@j₂, ...] where jₖ are the valid b ranks under isValid. A single rankOrPos helper resolves to a position variable on the pinned axis and a rank auxiliary on non-pinned axes. The pinned case collapses to the classic positional implication-chain form without a separate code path — no duplication of per-constraint positional encoders. Applied to next_to, not_next_to, left_of, before, exact_distance, between, and not_between. Removed badBinaryRankPairs. Also fixes a latent bug in the not_next_to predicate: requiring i !== j would have forbidden two cross-category values from sharing the same position, which is normal (AMO only governs within a category). Silence v8 coverage on the defensive "retry exhausted" throw in generator.ts — unreachable for supported grid sizes, matches the existing convention for the minimize-returns-empty failsafe. Tests: 8x8 generation 710ms → 188ms, full suite ~570ms. Coverage: 100% (was flaky on the retry-exhausted path).
- axisSuffix() walked grid.categories on every comparative deduction step to count ordered categories. Compute once in computeAxisTerms and store as multiAxis: boolean. - Fix previously flaky 99.92% coverage on generator.ts line 117 (difficulty-retry branch): the 'expert puzzles require contradiction' test had no seed — Math.random sometimes produced an expert puzzle on the first attempt, skipping the retry branch. Pin seed: 1 so at least one retry always occurs (seed 1's first-attempt natural difficulty is 'hard'). 30/30 runs at 100% coverage.
Add an assertion that seed 1's natural difficulty is non-expert. If RNG or difficulty scoring ever changes such that seed 1 produces expert on the first attempt, this test fails immediately rather than silently losing coverage of the difficulty-retry branch.
PR review fixes: - Extract pinnedAxis() and isPinnedAxis() to axis.ts so encoding.ts, deduce/constraints.ts, and deduce/state.ts can't drift on their three previously-local copies of grid.categories.find(c.ordered). - Fix randomSolution() to pin the first-ordered axis, matching what encodeBase pins. It previously used displayAxisCategory() which diverges when grid.displayAxis points at a non-first ordered category — the SAT-canonical solution would then be a permutation of randomSolution's solution. Functionally harmless (constraints are relational, uniqueness still holds) but a real divergence with no test coverage. Now they agree. - Add a test that uses displayAxis pointing at the second ordered category and asserts: uniqueness still holds, and the canonical solution pins Year (first ordered) at identity — locks in the invariant that displayAxis is a UI hint, not a SAT concern. Push back on: - SILENT_STEP.technique sentinel: comment is sufficient; adding "silent" to the public DeductionTechnique union pollutes a public type for a private implementation detail. - Unguarded channeling clauses: deliberate — channeling defines rank var semantics globally, not per-constraint.
Call out that solver/SAT concerns should use pinnedAxis, since the two functions diverge when grid.displayAxis points at a non-first ordered category.
PR review fixes: (1) RankVarAllocator footgun — seal on first varCeiling read. Throw if rankVar() is called after varCeiling is captured, preventing silent collisions with caller-allocated variables (e.g. activation literals). Cache hits still return existing vars. 9 new unit tests cover channeling direction, AMO, ALO, cache hits, allocation growth, and the seal contract. (3) isAxisValue false-positive risk — validate globally unique values in validateCategories(). The SAT variable mapping in createContext() was already keyed by value name across categories, so duplicates silently overwrote the earlier entry. isAxisValue had the same latent risk. Now explicitly rejected with a clear message at grid construction. (4) Extract topPositionVar(ctx) helper. Previously ctx.numValues * ctx.numPositions + 1 duplicated the layout assumption from variable(). Centralized so future position-var packing changes don't desync. Push back on: (2) SILENT_STEP.technique — already addressed; adding "silent" to the public DeductionTechnique union pollutes a public type for a private implementation detail. Callers only check !== null. (7) exact_distance predicate centralization — "not important" per review.
PR review fixes: (1) Binary ops (next_to, left_of, before, exact_distance) and same_position previously used 'knownA || knownB' for because-context, mentioning at most one operand even when both are known. Now joined with 'and' — matches the richer phrasing tryBetweenAxis already produces. (2) Document that sat.ts processUnitClauses() restructure is a v8 coverage quirk workaround (chained else-if was reported as a single branch). (4) Add defensive fallback in buildNudgeText for unknown DeductionTechnique values. Catches runtime breakage from persisted deduction traces that still reference the removed "direct" / "elimination" techniques. Test added with a cast to simulate a persisted trace. (5) Comment on describeKnown's 'possible.size < state.n' guard explaining why 'can only be anywhere' context gets suppressed. (3) PR description now explicitly calls out that any custom category not in the default set (Language, Instrument, Flower, Nationality, etc.) needs 'lowercase: true' to preserve old rendering.
axisSuffix previously returned ' on <Axis>' whenever the grid had multiple ordered categories, even when the constraint was on the pinned axis — chatty noise since the pinned axis is the implicit row anchor. Now the suffix only appears for non-pinned-axis constraints where disambiguation actually helps. Addresses PR review issue (6).
PR review fixes:
(1) tryExactDistance — when the axis has no unit, fall back to the
target axis's noun instead of state.terms.noun (which describes the
pinned axis). For a Year-axis exact_distance on a House-pinned grid,
we now emit "3 years" instead of "3 houses". Added a regression test
in constraints.test.ts that pins Rank values to House positions, so
rank-space deduction on the non-pinned Rank axis fires.
(2) sat.ts processUnitClauses() — revert to the cleaner
`if (UNDEF) { assign } else if (mismatch) { return false }` form.
v8 coverage now reports both branches correctly.
(3) Comment on isAxisValue in computeAxisTerms linking it to the
global-uniqueness invariant enforced by validateCategories. If the
invariant weakens, this lookup silently misclassifies cross-category
collisions.
Push back on:
- SILENT_STEP.technique "arbitrary placeholder" comment — already
clear; making it `unknown as DeductionStep` would sacrifice
type-safety for essentially cosmetic docs. Callers only check
`!== null`.
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.
Summary
lowercase?: booleanto categories. Proper nouns preserved by default.Breaking changes
atPosition()/notAtPosition()constraint factories removed. Migrate tosamePosition(value, axisValue)/notSamePosition(value, axisValue)whereaxisValueis the display-axis value at the desired position. Example:at_position/not_at_positionconstraint types removed fromConstraintunion. Any persisted JSON puzzles or serialized constraint arrays using these types must be migrated tosame_position/not_same_positionwith the display axis value asb."direct"/"elimination"deduction techniques removed fromDeductionTechniqueunion. Persisted deduction traces referencing these techniques no longer type-check. The demo'sbuildNudgeTextnow falls back to a generic hint when it encounters an unknown technique, so persisted traces won't crash at runtime — but consumers outside the demo that switch onDeductionTechniqueshould audit their fallback.Values in clue text preserve casing by default. Previously every value was lowercased in object position (
\"Alice owns the cat\"). Categories that need lowercasing (common nouns, adjectives) must now setlowercase: true. Default built-in categories (Color, Pet, Drink, Food, Hobby, Music, Sport) have this set. Any custom category not in that default set — including adjective-like ones such as Language, Instrument, Flower, Nationality — must opt in tolowercase: trueto keep the old rendering; otherwise values like "Italian" or "Violin" now appear capitalized in clue text. Custom categories with proper-noun values (ship names, place names) render correctly without configuration.Global value uniqueness enforced at grid construction.
validateCategoriesnow rejects grids where the same value name appears in two categories (the SAT variable mapping was already keyed by value name across categories — collisions silently overwrote entries before).Test plan