feat: variant-aware reduction paths with resolve_path#68
Conversation
Add data types for variant-aware reduction paths: - ReductionStep: a node with problem name and variant map - EdgeKind: either a registered Reduction (with overhead) or NaturalCast - ResolvedPath: sequence of steps connected by edges, with helper methods Also add Serialize derives to Monomial, Polynomial, and ReductionOverhead to support serialization of the new EdgeKind::Reduction variant. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…path Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Allow deprecated lookup_overhead calls in examples pending migration, revert unintended macro indentation change, and apply rustfmt to tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Describes the two-phase resolution approach (name-level discovery + variant-level resolve_path), KSat disambiguation, natural cast insertion, and the execution model. Removes the mistakenly created root design.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #68 +/- ##
==========================================
- Coverage 97.06% 96.76% -0.30%
==========================================
Files 187 185 -2
Lines 25150 25789 +639
==========================================
+ Hits 24412 24955 +543
- Misses 738 834 +96 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… types ResolvedPath, ReductionStep, and EdgeKind are actively used in tests and the KSat example, so the dead_code suppression is no longer needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces variant-aware reduction path resolution to address two key issues: overhead ambiguity when multiple reduction variants exist (e.g., KSat k=2 vs k=3), and automatic insertion of natural cast steps when variant relaxation is needed. The implementation adds a two-phase approach where name-level paths are first discovered, then resolved to variant-level paths with precise overhead and natural cast information.
Changes:
- Adds
resolve_pathmethod that lifts name-levelReductionPathinto variant-levelResolvedPathwith automatic natural cast insertion - Introduces
find_best_entryfor variant-awareReductionEntrylookup, resolving overhead ambiguity by selecting the most specific compatible entry - Deprecates
lookup_overhead/lookup_overhead_or_emptyin favor of extracting overhead from resolved paths
Reviewed changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/rules/graph.rs |
Adds ResolvedPath, ReductionStep, EdgeKind types and resolve_path/find_best_entry methods for variant-aware path resolution |
src/rules/mod.rs |
Exports new public types EdgeKind, ReductionStep, ResolvedPath |
src/rules/registry.rs |
Adds Serialize derive to ReductionOverhead for JSON export support |
src/polynomial.rs |
Adds Serialize derive to Monomial and Polynomial for JSON export support |
src/export.rs |
Deprecates lookup_overhead and lookup_overhead_or_empty with migration guidance |
src/rules/natural.rs |
Removes explicit impl_natural_reduction! invocation, now handled automatically by resolve_path |
examples/reduction_ksatisfiability_to_qubo.rs |
Migrates from lookup_overhead to resolve_path API demonstrating variant-aware overhead extraction |
src/unit_tests/rules/graph.rs |
Comprehensive tests for resolve_path, find_best_entry, variant disambiguation, and natural cast insertion |
src/unit_tests/rules/natural.rs |
Updates test to verify natural cast insertion via resolve_path |
src/unit_tests/export.rs |
Adds #[allow(deprecated)] to tests using deprecated functions |
examples/*.rs |
Adds #[allow(deprecated)] to examples pending incremental migration |
docs/src/design.md |
Adds comprehensive documentation on variant-aware path resolution algorithm and usage |
docs/plans/*.md |
Adds detailed design and implementation plans for the feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/rules/graph.rs
Outdated
| if current_variant != entry_source { | ||
| steps.push(ReductionStep { | ||
| name: src_name.to_string(), | ||
| variant: entry_source, | ||
| }); | ||
| edges.push(EdgeKind::NaturalCast); | ||
| } |
There was a problem hiding this comment.
The logic for inserting natural casts in resolve_path has a potential issue. When current_variant != entry_source, a natural cast is inserted. However, the code doesn't verify that the natural cast is actually valid - it should check is_variant_reducible(current_variant, entry_source) before inserting the cast. Without this check, incompatible variants could be incorrectly cast.
The find_best_entry method already ensures compatibility by checking is_variant_reducible(current_variant, entry_source) at line 723, which prevents incompatible entries from being returned. However, for code clarity and defensive programming, an explicit check or assertion before the natural cast insertion would make the invariant more obvious.
There was a problem hiding this comment.
Addressed: added a debug_assert!(is_variant_reducible(...)) before the natural cast insertion. As you noted, find_best_entry already guarantees compatibility, but the assertion makes the invariant explicit.
src/rules/graph.rs
Outdated
| /// A matched reduction entry: (source_variant, target_variant, overhead). | ||
| pub type MatchedEntry = ( | ||
| std::collections::BTreeMap<String, String>, | ||
| std::collections::BTreeMap<String, String>, | ||
| ReductionOverhead, | ||
| ); |
There was a problem hiding this comment.
The MatchedEntry type alias uses a tuple (BTreeMap<String, String>, BTreeMap<String, String>, ReductionOverhead) which makes the code less self-documenting. Consider defining a struct instead:
pub struct MatchedEntry {
pub source_variant: BTreeMap<String, String>,
pub target_variant: BTreeMap<String, String>,
pub overhead: ReductionOverhead,
}This would make call sites like let (source_var, target_var, overhead) = entry.unwrap() more explicit as let entry = entry.unwrap(); entry.source_variant... and improve code readability.
There was a problem hiding this comment.
Addressed: converted MatchedEntry from a tuple type alias to a named struct with source_variant, target_variant, and overhead fields. All usage sites updated.
Move the ~340-line inline script and inline styles from the reduction graph in introduction.md into separate docs/src/static/reduction-graph.js and docs/src/static/reduction-graph.css files. Update book.toml to load cytoscape.min.js and the new files via additional-js/additional-css. Key changes: - Tooltip element renamed from #tooltip to #cy-tooltip to avoid conflicts - clearPath() is now a local function with addEventListener instead of inline onclick handler - DOMContentLoaded guard ensures the script only runs when #cy exists - No behavior change; pure mechanical extraction Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Restructure tap handler so path selection takes priority when a source node is already selected, allowing variant nodes to participate in path finding while still supporting single-click variant edge filtering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix CDN URL for cytoscape-elk (correct path: /dist/cytoscape-elk.js) - Add explicit ELK layout registration with cose fallback when CDN unavailable - Fix expand/collapse to only show variant edges when both endpoints are visible - Move search input styles to external CSS file Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ant_params! macros Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace `KSatisfiability<const K: usize>` with `KSatisfiability<K: KValue>` using the new variant type system (K2, K3, KN types). Key changes: - KSatisfiability struct now uses PhantomData<K> with serde skip/bound - Reduction impls use concrete types (K2, K3, KN) instead of const generics since the #[reduction] proc macro requires concrete types - KSAT->SAT registered for KN (generic), K3, and K2 - SAT->KSAT registered for K3 (same as before) - KSAT->QUBO registered for K2 and K3 (same as before) - Variant IDs change from "2"/"3"/"N" to "K2"/"K3"/"KN" - Prelude exports K2, K3, KN, KValue, VariantParam, CastToParent Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace KColoring<const K: usize, G> with KColoring<K: KValue, G> to use type-level K parameters (K1, K2, K3, K4, KN) from the variant system. Key changes: - Add K1 and K4 types, extend hierarchy: K1 < K2 < K3 < K4 < KN - Update KColoring model to use KValue trait with PhantomData<K> - Migrate all reduction rules (SAT->KColoring, KColoring->QUBO, KColoring->ILP) using helper function + macro pattern for concrete type registrations - Add k_hierarchy to ReductionGraph built from VariantTypeEntry inventory for proper subtype checking of K-value variants - Update all tests, examples, and integration tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace all hand-written variant() implementations with the variant_params! macro across 19 problem files. Add VariantParam trait implementations for GridGraph<i32> and Triangular graph types. Propagate VariantParam bounds to generic ReductionResult impls and test helpers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…Entry Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ystem The unified VariantTypeEntry system now fully covers all graph and weight subtype registrations. Add VariantParam impls and VariantTypeEntry registrations for PlanarGraph and BipartiteGraph (the two marker types that were only in the old system), then remove GraphSubtypeEntry, WeightSubtypeEntry, GraphMarker trait, GraphSubtype trait, and the declare_graph_subtype!/declare_weight_subtype! macros. Rewrite graph_types tests to verify the new system. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…eneric rewriting Remove legacy variant utilities replaced by the new VariantParam trait system: - const_usize_str and short_type_name from src/variant.rs - collect_const_generic_names and rewrite_const_generics from proc macro - Simplify make_variant_fn_body to call Problem::variant() directly - Remove corresponding tests from unit_tests/variant.rs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… redesign Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Change SummaryThis PR is a comprehensive redesign of the variant system, spanning 36 commits across 191 files (+7141/-3106 lines). The work falls into four major areas: 1. Variant-Aware Reduction Path Resolution
2. Type-Level Variant Parameter System
3. Interactive Reduction Diagram (mdBook)
4. Cleanup
|
# Conflicts: # src/unit_tests/models/graph/kcoloring.rs # src/unit_tests/models/satisfiability/ksat.rs # src/unit_tests/rules/sat_coloring.rs # src/unit_tests/rules/sat_ksat.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 107 out of 107 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/rules/graph.rs
Outdated
| // Pick the most specific: if we already have a best, prefer the one | ||
| // whose source_variant is more specific (tighter fit) | ||
| let dominated = if let Some((ref best_source, _, _)) = best { | ||
| // New entry is more specific than current best? | ||
| self.is_variant_reducible(&entry_source, best_source) | ||
| || entry_source == *current_variant | ||
| } else { | ||
| true | ||
| }; | ||
|
|
||
| if dominated { | ||
| best = Some((entry_source, entry_target, entry.overhead())); |
There was a problem hiding this comment.
The logic for selecting the "most specific" entry in find_best_entry is potentially incorrect. At line 693, the check self.is_variant_reducible(&entry_source, best_source) would be true if entry_source is MORE restrictive than best_source. However, the comment says "New entry is more specific than current best?" which suggests we want to prefer the entry_source that is MORE restrictive (closer to current_variant). The condition should likely be inverted or the logic reconsidered. If is_variant_reducible(A, B) means "A can be reduced to B" (A is more specific), then this check correctly identifies when the new entry is more specific. But the || entry_source == *current_variant clause suggests exact matches should be preferred, which conflicts with the reducer logic. Please verify this selection logic is correct.
There was a problem hiding this comment.
The logic is correct. is_variant_reducible(A, B) means A ≤ B (A is more specific / a subtype of B). So is_variant_reducible(entry_source, best_source) is true when entry_source is more specific than best_source, which is exactly what we want — prefer the tighter fit. The || entry_source == *current_variant handles exact matches as a priority shortcut. Added a clarifying comment to the code.
src/variant.rs
Outdated
| impl_variant_param!(KN, "k", k: None); | ||
| impl_variant_param!(K4, "k", parent: KN, cast: |_| KN, k: Some(4)); | ||
| impl_variant_param!(K3, "k", parent: K4, cast: |_| K4, k: Some(3)); | ||
| impl_variant_param!(K2, "k", parent: K3, cast: |_| K3, k: Some(2)); | ||
| impl_variant_param!(K1, "k", parent: K2, cast: |_| K2, k: Some(1)); |
There was a problem hiding this comment.
The K-value hierarchy is defined in reverse order: K1 < K2 < K3 < K4 < KN. This means a problem with K=1 is considered more specific than K=2, which is counterintuitive. For subtype relationships, you typically want the more general type as the parent. Consider whether K1 should actually be the "most specific" and KN the "most general" (root). If the intent is that smaller K values can be used where larger K values are expected, the hierarchy should be reversed: KN (root) with K4 < K3 < K2 < K1 as children, or reconsider the parent relationships.
There was a problem hiding this comment.
The hierarchy is intentional: K1 < K2 < K3 < K4 < KN means "K1-SAT is a special case of K2-SAT". Every 1-clause can be treated as a 2-clause (by repeating a literal), so a K2-SAT solver handles K1-SAT inputs. The parent direction represents generalization: KN (arbitrary K) is the most general, K1 the most specific. This matches the graph hierarchy pattern (KingsSubgraph < UnitDiskGraph < SimpleGraph).
- Remove GridGraph/Grid type entirely; MappingResult stores positions, node_weights, grid_dimensions, and kind (GridKind enum) directly - Add KingsSubgraph and TriangularSubgraph as public graph types that compute edges on-the-fly from geometry - Remove backward-compat aliases from unitdiskmapping/mod.rs; all consumers now use canonical ksg:: and triangular:: paths - Flatten K value hierarchy: K1..K5 are direct children of KN with no chain between them (k-SAT/k-coloring with different k are independent problem classes) - Add K5 type - Regenerate static JSON exports with flat visualization format - Add variant hierarchy diagram (Typst → SVG) to design docs - Remove irrelevant plan files from this branch Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
resolve_pathto lift name-levelReductionPathinto variant-levelResolvedPath, threading graph/weight/constant variants through each reduction step and insertingNaturalCastedges where subtype relaxation is neededfind_best_entryfor variant-awareReductionEntrylookup, resolving the KSat overhead ambiguity (k=2 vs k=3 now return distinct overheads)lookup_overhead/lookup_overhead_or_emptyin favor of extracting overhead from the resolved pathimpl_natural_reduction!invocation — natural casts are now computed from subtype hierarchiesTest plan
make test clippypassesResolvedPath,find_best_entry, variant disambiguation (KSat k=2 vs k=3), natural cast insertion (GridGraph→SimpleGraph, Triangular→SimpleGraph)resolve_pathAPI#[allow(deprecated)]— to be migrated incrementallySee
design.mdfor full design rationale.🤖 Generated with Claude Code