Fix #213: Add MinimumFeedbackArcSet model#614
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #614 +/- ##
==========================================
+ Coverage 96.77% 96.79% +0.01%
==========================================
Files 232 234 +2
Lines 30494 30743 +249
==========================================
+ Hits 29512 29757 +245
- Misses 982 986 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add the MinimumFeedbackArcSet problem model for finding minimum-size arc subsets whose removal makes a directed graph acyclic. This includes: - New DirectedGraph topology type (wrapping petgraph DiGraph) - MinimumFeedbackArcSet model with BruteForce solver support - Full CLI support (create, solve, serialize) with --arcs flag - Paper documentation with formal definition and example - Comprehensive unit tests (12 tests covering creation, evaluation, solver, serialization, and edge cases) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
Author
Implementation SummaryChanges
Deviations from Plan
Open Questions
|
Collaborator
Author
Implementation SummaryChanges
Deviations from Plan
Open Questions
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
Author
Implementation SummaryChanges
Deviations from Plan
Open Questions
|
There was a problem hiding this comment.
Pull request overview
Adds support for a directed-graph topology and introduces the Minimum Feedback Arc Set (MFAS) optimization model, integrating it into the library exports, CLI (create/dispatch), unit tests, and paper docs.
Changes:
- Introduce
DirectedGraphtopology type (petgraph-backed) and corresponding unit tests. - Add
MinimumFeedbackArcSetmodel + tests, and re-export it through the crate modules/prelude. - Extend CLI to recognize/serialize/deserialize/create MFAS instances via a new
--arcsflag; update docs/paper to include MFAS.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/topology/mod.rs |
Registers and re-exports the new DirectedGraph type. |
src/topology/directed_graph.rs |
Implements a directed graph wrapper and DAG-check helper used by MFAS. |
src/models/graph/mod.rs |
Adds the MFAS module and re-export. |
src/models/graph/minimum_feedback_arc_set.rs |
Implements the MFAS optimization problem and registers it in the schema/variants system. |
src/models/mod.rs |
Re-exports MFAS at the models module boundary. |
src/lib.rs |
Re-exports MFAS through the crate prelude. |
src/unit_tests/topology/directed_graph.rs |
Adds unit tests for DirectedGraph. |
src/unit_tests/models/graph/minimum_feedback_arc_set.rs |
Adds unit tests for MFAS behavior/solver correctness/serialization. |
src/unit_tests/trait_consistency.rs |
Ensures MFAS participates in trait consistency + direction tests. |
problemreductions-cli/src/problem_name.rs |
Adds an alias mapping for minimumfeedbackarcset. |
problemreductions-cli/src/dispatch.rs |
Adds load/serialize dispatch arms for MFAS. |
problemreductions-cli/src/commands/create.rs |
Adds --arcs parsing and MFAS creation path. |
problemreductions-cli/src/cli.rs |
Documents MFAS flags and adds arcs to CLI args. |
examples/detect_isolated_problems.rs |
Formatting-only change. |
docs/paper/references.bib |
Adds MFAS-related references. |
docs/paper/reductions.typ |
Adds MFAS definition to the paper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let num_arcs = graph.num_arcs(); | ||
| if config.len() != num_arcs { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
evaluate() / is_valid_fas() currently treat any non-zero value in config as “removed” (since kept_arcs uses x == 0), but the objective counts only entries equal to 1. This means configs containing values like 2 could be considered valid while under-counting the removed arcs. Suggest either (a) rejecting configs with values > 1 as invalid, or (b) consistently interpreting “removed” as x != 0 and counting accordingly.
Suggested change
| } | |
| } | |
| // Ensure configuration values are binary (0 = kept, 1 = removed) | |
| if config.iter().any(|&x| x > 1) { | |
| return false; | |
| } |
| } | ||
|
|
||
| crate::declare_variants! { | ||
| MinimumFeedbackArcSet => "2^num_vertices", |
There was a problem hiding this comment.
The declared variant complexity is "2^num_vertices", but this model’s configuration space is binary per arc (dims() is 2 repeated num_arcs() times). The complexity metadata should likely be expressed in terms of num_arcs to match the actual variable count.
Suggested change
| MinimumFeedbackArcSet => "2^num_vertices", | |
| MinimumFeedbackArcSet => "2^num_arcs", |
Comment on lines
+117
to
+132
| /// Check if the subgraph induced by keeping only the given arcs is acyclic (a DAG). | ||
| /// | ||
| /// `kept_arcs` is a boolean slice of length `num_arcs()`, where `true` means the arc is kept. | ||
| pub fn is_acyclic_subgraph(&self, kept_arcs: &[bool]) -> bool { | ||
| let n = self.num_vertices(); | ||
| let arcs = self.arcs(); | ||
|
|
||
| // Build adjacency list for the subgraph | ||
| let mut adj = vec![vec![]; n]; | ||
| let mut in_degree = vec![0usize; n]; | ||
| for (i, &(u, v)) in arcs.iter().enumerate() { | ||
| if kept_arcs[i] { | ||
| adj[u].push(v); | ||
| in_degree[v] += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
is_acyclic_subgraph() indexes kept_arcs[i] for every arc, but does not validate kept_arcs.len() == num_arcs(). Since this is a public API, a mismatched slice length will panic with an out-of-bounds access. Consider adding an explicit length check (return false or panic with a clearer message) before iterating.
Comment on lines
86
to
92
| "QUBO" => "--matrix \"1,0.5;0.5,2\"", | ||
| "SpinGlass" => "--graph 0-1,1-2 --couplings 1,1", | ||
| "KColoring" => "--graph 0-1,1-2,2-0 --k 3", | ||
| "Factoring" => "--target 15 --m 4 --n 4", | ||
| "MinimumFeedbackArcSet" => "--arcs \"0>1,1>2,2>0\"", | ||
| _ => "", | ||
| } |
There was a problem hiding this comment.
The pred create schema-driven help will list parameters based on ProblemSchemaEntry field names. For MinimumFeedbackArcSet, that means users will see --graph (since the JSON field is graph), but the actual CLI input flag implemented here is --arcs. Consider special-casing DirectedGraph / MinimumFeedbackArcSet in print_problem_help() (or type_format_hint()) so the parameters section advertises --arcs instead of --graph.
- Fix evaluate/is_valid_fas consistency: use x != 0 for counting removed arcs - Fix complexity from 2^num_vertices to 2^num_arcs (matches config space) - Add length validation assert to is_acyclic_subgraph - Add DirectedGraph to type_format_hint and show --arcs in schema help Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add 'FAS' as a short alias for MinimumFeedbackArcSet (matching FVS pattern) - Support --num-vertices flag for creating graphs with isolated vertices - Update CLI help text Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Review Pipeline Report
🤖 Generated by review-pipeline |
…012) The best known exact algorithm for Minimum Feedback Arc Set uses DP over vertex subsets in O*(2^n) time, not brute force over arcs. Add citation to Bodlaender, Fomin, Koster, Kratsch, Thilikos (2012) in both the paper and references.bib. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add generic W parameter matching FVS pattern (WeightElement + VariantParam) - Evaluate sums arc weights instead of counting (supports weighted FAS) - Add weights(), set_weights(), is_weighted() accessors - Deduplicate arc parsing: extract parse_directed_graph() shared by FAS/FVS - Add parse_arc_weights() for FAS --weights CLI support - Add weighted test case (high-weight arc avoided by solver) - Register MinimumFeedbackArcSet<i32> variant in declare_variants! - Update dispatch.rs to use MinimumFeedbackArcSet<i32> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Resolve merge conflicts by combining MinimumFeedbackArcSet additions with new models from main (RuralPostman, SubgraphIsomorphism, PartitionIntoTriangles, etc.) in shared files: mod.rs, lib.rs, create.rs, dispatch.rs, and reductions.typ.
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
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
Adds the
MinimumFeedbackArcSetproblem model, including a newDirectedGraphtopology type as a prerequisite.Fixes #213