Skip to content

Fix #242: [Model] MixedChinesePostman#729

Merged
GiggleLiu merged 5 commits intomainfrom
issue-242
Mar 21, 2026
Merged

Fix #242: [Model] MixedChinesePostman#729
GiggleLiu merged 5 commits intomainfrom
issue-242

Conversation

@GiggleLiu
Copy link
Copy Markdown
Contributor

Summary

Add the implementation plan for MixedChinesePostman and execute it on the issue branch.

Fixes #242

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 21, 2026

Codecov Report

❌ Patch coverage is 94.09283% with 28 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@3ac6795). Learn more about missing BASE report.
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/models/graph/mixed_chinese_postman.rs 92.48% 20 Missing ⚠️
src/topology/mixed_graph.rs 90.36% 8 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #729   +/-   ##
=======================================
  Coverage        ?   97.60%           
=======================================
  Files           ?      409           
  Lines           ?    50893           
  Branches        ?        0           
=======================================
  Hits            ?    49676           
  Misses          ?     1217           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@GiggleLiu
Copy link
Copy Markdown
Contributor Author

Implementation Summary

Changes

  • Added a new MixedGraph topology plus the MixedChinesePostman model, including exact evaluation logic, example-db registration, and unit coverage for the YES/NO issue fixtures.
  • Extended the CLI to create MixedChinesePostman instances with --graph, --arcs, --edge-weights, --arc-costs, and --bound, and added CLI tests and help text.
  • Added paper coverage for Mixed Chinese Postman, including bibliography entries and a worked example figure.

Deviations from Plan

  • While reviewing the new topology, I found and fixed an order-sensitivity bug in MixedGraph::has_edge; the final implementation includes a regression test for reversed undirected-edge input order.
  • Repo-wide fmt-check on this toolchain also reformatted two existing AcyclicPartition files, so those mechanical formatting diffs are included to keep make check green.

Open Questions

  • None.

@GiggleLiu
Copy link
Copy Markdown
Contributor Author

Agentic Review Report

Structural Check

Structural Completeness

# Check Status
1 Model file exists PASS
2 inventory::submit! present PASS
3 Serialize / Deserialize derive PASS
4 Problem trait impl PASS
5 SatisfactionProblem impl PASS
6 Test link via #[path = ...] PASS
7 Model test file exists PASS
8 Test file has >= 3 tests PASS
9 Registered in src/models/graph/mod.rs PASS
10 Re-exported in src/models/mod.rs PASS
11 declare_variants! entry exists PASS
12 CLI alias resolution support PASS — registry-driven alias; pred show MCPP resolves to MixedChinesePostman
13 CLI create support PASS
14 Canonical model example registered PASS
15 Paper display-name entry PASS
16 Paper problem-def block PASS
17 Blacklisted auto-generated files absent PASS

Build Status

  • make test: PASS
  • make clippy: PASS

Semantic Review

  • evaluate() correctness: ISSUE — src/models/graph/mixed_chinese_postman.rs:149-255 fixes one direction for each undirected edge, then solves a directed CPP on that oriented graph. That is stricter than Mixed Chinese Postman: repeated traversals of an undirected edge may use either direction. Counterexample: with V={0,1}, A=∅, E={{0,1}}, B=2, the walk 0-1-0 is valid, but both configs evaluate false.
  • dims() correctness: OK — src/models/graph/mixed_chinese_postman.rs:230 returns [2; num_edges], matching the issue’s revised finite encoding.
  • Feasibility / connectivity: ISSUE — src/models/graph/mixed_chinese_postman.rs:239-242 requires the once-oriented digraph to be strongly connected on all vertices. Isolated zero-degree vertices therefore make the instance false even when the non-isolated part admits a valid closed walk.
  • Size getter consistency: OK — src/models/graph/mixed_chinese_postman.rs:129-137 exposes num_vertices, num_arcs, and num_edges, consistent with the declared complexity expression.
  • Weight handling: OK — src/models/graph/mixed_chinese_postman.rs:114-127 uses inherent accessors and separate arc/edge weight vectors as required.
  • MixedGraph topology: OK — src/topology/mixed_graph.rs:27-107 validates endpoints and treats undirected edges as unordered for membership/equality.

Issue Compliance

# Check Status
1 Problem name matches issue OK
2 Mathematical definition matches ISSUE — src/models/graph/mixed_chinese_postman.rs:149-255 implements an orientation-fixed directed-postman check, not the mixed-graph definition from issue #242.
3 Problem type (opt/sat) matches OK
4 Type parameters match OK
5 Configuration space matches OK
6 Feasibility check matches ISSUE — src/models/graph/mixed_chinese_postman.rs:239-242 rejects feasible mixed-postman instances whenever the once-oriented digraph is not strongly connected.
7 Objective function matches ISSUE — src/models/graph/mixed_chinese_postman.rs:249-255 minimizes augmentation cost only in the oriented digraph, so it cannot exploit reverse-direction reuse of undirected edges allowed by the issue definition.
8 Complexity matches OK — src/models/graph/mixed_chinese_postman.rs:264-267 matches 2^num_edges * num_vertices^3.
9 Size fields exposed ISSUE — the model does not register ProblemSizeFieldEntry, so pred show MixedChinesePostman shows no size fields despite issue #242 requiring them.

Summary

  • 17/17 structural checks passed.
  • 5/9 issue-compliance checks passed.
  • Main structural findings: incorrect mixed-postman semantics; over-strict connectivity; missing size-field metadata.

Quality Check

Design Principles

  • DRY: OK — no material duplication stood out as the source of risk in this PR.
  • KISS: ISSUE — the new model simplifies Mixed Chinese Postman into “choose one direction for each undirected edge, then solve a directed postman subproblem,” which is not equivalent to the actual problem and rejects valid yes-instances. See src/models/graph/mixed_chinese_postman.rs:39,149,234.
  • HC/LC: OK — the helpers are split out reasonably; the main problem here is semantic correctness, not module boundaries.

HCI

  • Error messages: OK — the new MixedChinesePostman diagnostics are actionable and include recovery hints.
  • Discoverability: ISSUE — the CLI advertises --graph and --arcs, but the valid E = ∅ special case is still hidden behind the non-obvious --graph "" --num-vertices N path because parse_mixed_graph() always routes through parse_graph(). See problemreductions-cli/src/commands/create.rs:839,3864,4824.
  • Consistency: OK — help text, examples, and parser names line up for --graph, --arcs, --edge-weights, and --arc-costs.
  • Least surprise: OK — for the supported mixed-edge path, the CLI behavior matches the documented flags.
  • Feedback: OK — pred create either emits structured JSON or a targeted error with usage.

Test Quality

  • Naive test detection: ISSUE — the new tests miss the adversarial cases that distinguish true mixed-walk semantics from the orientation-only approximation.
  • src/unit_tests/models/graph/mixed_chinese_postman.rs:45 only covers orientation-friendly examples; it never tests the smallest valid mixed instance V={0,1}, A=∅, E={{0,1}}, B=2, which the current implementation rejects even though the closed walk 0-1-0 exists.
  • problemreductions-cli/tests/cli_tests.rs:2055 checks JSON shape and one input error, but does not add a solve round-trip or any adversarial case exercising the actual mixed-graph semantics.

Issues

Critical (Must Fix)
  • The added MixedChinesePostman model is not a correct implementation of Mixed Chinese Postman. It assumes each undirected edge must be oriented exactly once up front and then requires the resulting digraph to be strongly connected. That is strictly stronger than the real problem, where an undirected edge may be traversed in either direction as needed within the closed walk. Concrete counterexample: on two vertices with one undirected edge of length 1 and bound 2, the walk 0-1-0 is valid, but both configs are rejected here because neither single-edge orientation is strongly connected. The paper text also repeats the same incorrect semantics at docs/paper/reductions.typ:3587.
Important (Should Fix)
  • The new test coverage does not guard the semantic boundary that the implementation gets wrong. Both the model tests and CLI tests stay within examples that already fit the orientation-only approximation, so this bug would survive with all new tests green.
Minor (Nice to Have)
  • pred create MixedChinesePostman makes the pure-directed special case harder to discover than it should be, because --graph is still effectively mandatory even when the undirected edge set is empty.

Summary

  • CriticalMixedChinesePostman implements a stricter orientation problem instead of true Mixed Chinese Postman, rejecting valid yes-instances.
  • Important — the added tests do not cover the adversarial mixed-only cases that would expose the semantic bug.
  • Minor — the CLI does not clearly surface the valid E = ∅ construction path for MixedChinesePostman.

Agentic Feature Tests

Area Result Notes
Discoverability Partial pred list exposed MixedChinesePostman and alias MCPP; pred show MixedChinesePostman worked, but only surfaced the i32 variant.
Setup Yes pred built successfully from an isolated temp copy of the PR worktree.
Functionality Partial Example creation and custom creation both worked. Solving worked with --solver brute-force; bare pred solve <instance> failed.
Expected vs Actual Partial The paper/docs imply create-then-solve should work directly. Actual behavior requires --solver brute-force.

Commands exercised:

  • pred list
  • pred show MixedChinesePostman
  • pred create MixedChinesePostman
  • pred create --example MixedChinesePostman
  • pred solve <instance>
  • pred solve <instance> --solver brute-force
  • pred create MixedChinesePostman --graph 0-2,1-3,0-4,4-2 --arcs "0>1,1>2,2>3,3>0" --edge-weights 2,3,1,2 --arc-costs 2,3,1,4 --bound 24

Observed results:

  • pred list showed both MixedChinesePostman/One and MixedChinesePostman/i32 *, alias MCPP.
  • pred show MixedChinesePostman showed description, fields, complexity, and no reductions.
  • pred create MixedChinesePostman printed problem-specific help and example usage.
  • pred create --example MixedChinesePostman succeeded without needing /i32.
  • Both example and custom instances solved successfully with --solver brute-force, returning evaluation: true and a satisfying solution.

Issues found:

  • confirmed medium: the paper example tells users to run bare pred solve mixed-chinese-postman.json at docs/paper/reductions.typ:3596, but that command fails in this worktree with “No reduction path from MixedChinesePostman to ILP... Try --solver brute-force.” Recommended fix: update the documented command to include --solver brute-force, or make pred solve fall back to brute-force when no ILP path exists.
  • confirmed low: the CLI guide says pred show displays all variants of a problem at docs/src/cli.md:201, but pred show MixedChinesePostman only showed MixedChinesePostman/i32, even though pred list showed both MixedChinesePostman/One and MixedChinesePostman/i32. Recommended fix: either make pred show enumerate both variants for this model, or narrow the docs if show is only meant to display the default/canonical variant.
  • not reproducible in current worktree: requiring an explicit variant suffix for the example path. pred create --example MixedChinesePostman worked without /i32.

Doc/help friction:

  • The model is discoverable from CLI help, but there is no short model-specific walkthrough outside the paper entry.
  • The create help uses a bound of 24, while the paper narrative example describes a yes-instance at 22; not blocking, but it makes it less clear which example is canonical.

Additional Reproductions

  • confirmed critical: a hand-written instance with V={0,1}, A=∅, E={{0,1}}, and B=2 is rejected by the shipped evaluator even though the closed walk 0-1-0 is valid. Reproduction from the PR worktree:
    • pred solve /tmp/review-pipeline-729/mcpp_single_edge.json --solver brute-forceError: No solution found
    • pred evaluate /tmp/review-pipeline-729/mcpp_single_edge.json --config 0false
    • pred evaluate /tmp/review-pipeline-729/mcpp_single_edge.json --config 1false
  • confirmed important: the CLI cannot create the valid A = ∅ special case from flags. pred create MixedChinesePostman --graph 0-1 --arcs '' --edge-weights 1 --bound 2 fails with Invalid arc '', so the documented mixed-graph domain is not fully constructible through pred create.
  • confirmed important: malformed MixedGraph JSON panics the CLI instead of producing an input error. A crafted instance with num_vertices = 2 and edge [0,2] panics in src/topology/directed_graph.rs:61; src/topology/mixed_graph.rs:14 derives Deserialize directly and bypasses endpoint validation.

Generated by review-pipeline

GiggleLiu and others added 2 commits March 21, 2026 22:15
…ed edges

The previous implementation only allowed traversals in the chosen
orientation direction, rejecting valid MCPP solutions. The real Mixed
Chinese Postman allows undirected edges to be traversed in either
direction during the walk. Fix by building the available-arc set with
both directions for connectivity checks and shortest-path computation,
while keeping oriented-only arcs for degree imbalance.

Also fix paper pred solve command to include --solver brute-force
(no ILP reduction path exists for this model).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@GiggleLiu GiggleLiu mentioned this pull request Mar 21, 2026
3 tasks
@GiggleLiu GiggleLiu merged commit a01930d into main Mar 21, 2026
3 checks passed
@GiggleLiu GiggleLiu deleted the issue-242 branch April 12, 2026 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Model] MixedChinesePostman

1 participant