Add enumeration-safe preprocessing mode (#43)

[mhoangvslev]

Summary

Closes #43. Implements the "enumeration-safe preprocessing mode" (option (a)) agreed on in the issue discussion: get_optimal_solutions() (#41/#42) requires preprocess=False because the default reduction pipeline can silently collapse tied-optimal alternatives before the ILP ever runs — degree-2 contraction against a same-cost parallel edge, node replacement, and the adjacent-terminal/Nearest-Vertex/Short-Links terminal-contraction tests each pick one witness among possibly several tied ones and structurally erase the rest.

An opt-in enumeration_safe=True constructor kwarg now restricts preprocess_graph() to reductions proven to preserve the complete set of optima, not merely the optimal value:

• Special distance, long-edge and bound-based deletions, and non-terminal degree-1 removal, are already exact (strict inequalities) and unaffected.
• Degree-2 contraction skips a node instead of contracting it when the contracted path ties an existing parallel edge (the A-B-C example from the issue).
• Node replacement (pseudo-elimination) is disabled outright.
• Of the terminal-contraction tests, only the forced degree-1 case still fires — adjacent-terminal, Nearest-Vertex and Short-Links are skipped.

get_optimal_solutions() now accepts preprocess=True when enumeration_safe=True, and back-maps the reduced-graph solution (adding the fixed-edge cost back to the objective) for that path — previously dead code, since preprocess=True was always rejected before reaching it.

Test plan

• pytest — full suite passes (736 tests), including new tests/test_enumeration_safe.py covering the diamond-graph tie from the issue end-to-end through get_optimal_solutions(), plus per-reduction checks that node replacement / adjacent-terminal / NV / SL are disabled while degree-1 terminal contraction and the strict deletion tests are unaffected.
• black/flake8/mypy — no new issues introduced versus the pre-existing baseline.
• Docs build cleanly (sphinx-build); solvers.md and performance.md updated to document the new flag.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
steinerpy/graph_reducer.py	88.88%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[berendmarkhorst]

Hi! Thanks for implementing this! I like the overall approach, especially keeping the strict deletion tests enabled while disabling or adjusting reductions that only preserve one optimum. The documentation and per-reduction tests are also helpful.

I tested the current implementation and found two correctness issues that should be fixed before merging:

1. Degree-constrained instances can become incorrectly feasible

max_degree disables the heavy reductions, but the degree-1/degree-2 structural reductions still run. These reductions do not necessarily preserve the degree constraints of the original graph.

For example:

g = nx.Graph()
g.add_weighted_edges_from([
    ("A", "B", 1),
    ("B", "C", 1),
])

raw = SteinerProblem(
    g,
    [["A", "C"]],
    preprocess=False,
    max_degree=1,
).get_optimal_solutions(limit=10)

safe = SteinerProblem(
    g,
    [["A", "C"]],
    preprocess=True,
    enumeration_safe=True,
    max_degree=1,
).get_optimal_solutions(limit=10)

print(len(raw), raw.exhausted)    # 0, True
print(len(safe), safe.exhausted)  # 1, True
print(list(safe)[0].edges)        # A-B, B-C

The original problem is infeasible because B must have degree 2. Preprocessing contracts B, solves the reduced A-C edge with maximum degree 1, and then maps it back to an infeasible original solution.

The simplest solution may be to reject preprocess=True for degree-constrained enumeration unless the reductions are made degree-aware.

2. da_reduce=True reintroduces unsafe degree-2 contractions

After preprocess_graph(..., enumeration_safe=True), the constructor can still call reduce_graph_with_dual_ascent(). Its reduced-cost deletions are strict and therefore safe, but its subsequent cascade calls _structural_fixpoint() without forwarding enumeration_safe=True.

This can again erase tied optima:

g = nx.Graph()
g.add_weighted_edges_from([
    ("A", "D", 2),
    ("A", "B", 1),
    ("B", "D", 1),
    ("A", "X", 1),
    ("B", "X", 100),
])

kwargs = dict(
    preprocess=True,
    enumeration_safe=True,
    heavy=False,
    contract_terminals=False,
)

safe = SteinerProblem(
    g, [["A", "D", "X"]], **kwargs
).get_optimal_solutions(limit=10)

safe_da = SteinerProblem(
    g, [["A", "D", "X"]], da_reduce=True, **kwargs
).get_optimal_solutions(limit=10)

print(len(safe), safe.exhausted)        # 2, True
print(len(safe_da), safe_da.exhausted)  # 1, True

Here, dual-ascent reduction deletes the expensive B-X edge. Its structural cascade then contracts B, even though A-B-D ties the existing A-D edge. The enumeration_safe flag should therefore be propagated into the dual-ascent structural cascade.

While testing, I also found a broader, pre-existing ambiguity in how get_optimal_solutions() handles redundant zero-cost edges, even with preprocess=False. I have opened a separate issue about that because it is not introduced by this PR. However, it affects the unconditional claim that enumeration_safe=True preserves the complete set of optima. Until the broader semantics are resolved, I suggest requiring strictly positive edge weights for enumeration-safe preprocessing and validating this explicitly.

Two smaller points:

• The new example in performance.md calls get_solution() rather than get_optimal_solutions(), so it does not actually demonstrate the new use case.
• Since this is a user-facing feature and every merge creates a release, it would be good to add it to the Unreleased section of CHANGELOG.md.

The full test suite and CI pass, and the main approach appears sound for unconstrained instances with strictly positive edge costs. Could you address the cases above and add regression tests comparing the enumeration-safe pool with preprocess=False or the brute-force oracle? After that, I would be happy to take another look.

[mhoangvslev]

Thanks for the thorough review — both correctness issues reproduced and are fixed:

1. max_degree/hop_limit: get_optimal_solutions() now raises NotImplementedError when preprocess=True is combined with either modifier, since the structural degree-1/degree-2 fixpoint isn't degree/hop-aware and always runs regardless. Your exact repro now raises instead of silently returning the infeasible 1-solution pool.
2. da_reduce cascade: reduce_graph_with_dual_ascent() now accepts and forwards enumeration_safe into its own _structural_fixpoint() call. Your repro now returns 2 tied solutions with da_reduce=True, matching without it.

Also added, per your suggestion:

• enumeration_safe=True now requires strictly positive edge weights (ValueError otherwise), sidestepping the zero-cost-edge ambiguity you split into a separate issue.
• Fixed the performance.md example to actually call get_optimal_solutions().
• Added a CHANGELOG.md entry under Unreleased.
• New regression tests: both reported repros, the weight validation, and — as requested — randomized comparisons of the enumeration-safe pool against a brute-force oracle and against preprocess=False across 10 seeded random graphs each.

Pushed as two new commits (5d7ee5d, a0eddcd). Full suite (754 tests) still passes.

[berendmarkhorst]

Hi! Thanks for updating this. The implementation now addresses my earlier review points, and the overall approach looks good.

Since PR #46 has been merged, this PR currently has conflicts in steinerpy/objects.py and docs/source/guide/solvers.md. Could you rebase it onto the latest main and retain both:

• PR Optimize Dreyfus-Wagner growth and fix optimal-solution enumeration #46’s inclusion-minimal enumeration semantics, zero-cost pruning, stronger no-good cuts, and empty-solution handling; and
• this PR’s enumeration_safe preprocessing, solution back-mapping, fixed-cost correction, and restrictions.

There is also one small documentation cleanup: some comments and the test for non-positive weights still describe zero-cost enumeration as an unresolved, out-of-scope ambiguity. PR #46 has now resolved that ambiguity by defining solutions as inclusion-minimal. The strictly-positive-weight restriction for enumeration_safe=True can remain, but its rationale should be that the preprocessing-preservation argument currently assumes positive weights—not that zero-cost enumeration itself is undefined.

I tested a local merge with these conflicts resolved, and the complete test suite passed. Once the branch is rebased and the outdated wording is updated, this looks ready to merge.

[mhoangvslev]

I just resolved the conflicts and it should work now.