Skip to content

Fix #110: Add LongestCommonSubsequence model and LCS to ILP reduction#598

Merged
GiggleLiu merged 9 commits intomainfrom
issue-110-lcs-to-ilp
Mar 13, 2026
Merged

Fix #110: Add LongestCommonSubsequence model and LCS to ILP reduction#598
GiggleLiu merged 9 commits intomainfrom
issue-110-lcs-to-ilp

Conversation

@zazabap
Copy link
Copy Markdown
Collaborator

@zazabap zazabap commented Mar 12, 2026

Summary

Adds the LongestCommonSubsequence problem model and implements the LCS-to-ILP reduction using the match-pair formulation from Blum et al. (2021).

Since the LCS model (issue #108, PR #170) has a deleted branch, this PR implements both the model and the reduction rule together.

Fixes #110

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 12, 2026

Codecov Report

❌ Patch coverage is 98.72774% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.63%. Comparing base (cc87cf0) to head (1ee99a4).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/rules/longestcommonsubsequence_ilp.rs 94.04% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #598      +/-   ##
==========================================
+ Coverage   96.61%   96.63%   +0.02%     
==========================================
  Files         218      222       +4     
  Lines       29352    29745     +393     
==========================================
+ Hits        28357    28745     +388     
- Misses        995     1000       +5     

☔ 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.

zazabap and others added 2 commits March 12, 2026 12:17
Add the LongestCommonSubsequence problem model (also addresses #108) and
implement the match-pair ILP formulation from Blum et al. (2021) for
reducing LCS to Integer Linear Programming.

Model: binary selection over shortest string positions, maximizing common
subsequence length across all input strings.

Reduction: for 2-string case, creates binary variables for character match
pairs with assignment and no-crossing constraints. Objective maximizes
matched pairs.

Includes CLI registration (create, dispatch, alias), unit tests with
closed-loop verification, example program, and paper entries.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@zazabap
Copy link
Copy Markdown
Collaborator Author

zazabap commented Mar 12, 2026

Implementation Summary

Changes

  • New model: src/models/misc/longest_common_subsequence.rs -- LongestCommonSubsequence problem with binary selection over shortest string, OptimizationProblem with Metric = SolutionSize<i32>, Direction::Maximize
  • New rule: src/rules/longestcommonsubsequence_ilp.rs -- Match-pair ILP formulation (Blum et al. 2021) with assignment and no-crossing constraints
  • Unit tests: Model tests (basic, serialization, brute force, 3-string case) + Reduction tests (closed-loop, edge cases)
  • Example: examples/reduction_longestcommonsubsequence_to_ilp.rs with ABAC/BACA instance
  • CLI: Registered in dispatch, problem_name (alias: LCS), create (--strings flag)
  • Paper: Added problem-def, reduction-rule, and bibliography entries (maier1978, blum2021)
  • Module registration: misc/mod.rs, models/mod.rs, lib.rs prelude, rules/mod.rs

Deviations from Plan

Open Questions

  • The reduction handles the 2-string case only (as specified in the issue). For k>2 strings, the brute-force solver works but the ILP reduction only uses the first two strings

The reduction test file needs Problem trait for evaluate() calls
and Solver trait for BruteForce::find_best() calls.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds the LongestCommonSubsequence (LCS) problem model and implements a reduction from LCS to ILP using the match-pair formulation from Blum et al. (2021). It covers both the model (originally PR #170 whose branch was deleted) and the reduction rule (issue #110) in a single PR.

Changes:

  • New LongestCommonSubsequence model with binary config over shortest string, subsequence feasibility checking, and brute-force solvability.
  • New LCS-to-ILP reduction using match-pair binary variables with one-to-one matching and order-preservation (no-crossing) constraints, scoped to 2-string instances.
  • Full integration: CLI dispatch/create/alias, module registration, prelude export, documentation in reductions.typ, bibliography entries, example, and comprehensive tests.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/models/misc/longest_common_subsequence.rs New LCS model: struct, new(), evaluate(), dims(), problem registration
src/rules/longestcommonsubsequence_ilp.rs LCS→ILP reduction: match-pair formulation, constraints, solution extraction
src/rules/mod.rs Register the new reduction module
src/models/misc/mod.rs Register the new model module
src/models/mod.rs Re-export LongestCommonSubsequence
src/lib.rs Add LongestCommonSubsequence to prelude
problemreductions-cli/src/dispatch.rs Add LCS to load_problem() and serialize_any_problem()
problemreductions-cli/src/commands/create.rs Add CLI --strings flag handling for LCS creation
problemreductions-cli/src/cli.rs Add --strings CLI argument and help text
problemreductions-cli/src/problem_name.rs Add "LCS" alias
src/unit_tests/models/misc/longest_common_subsequence.rs 14 unit tests for the model
src/unit_tests/rules/longestcommonsubsequence_ilp.rs 7 unit tests for the reduction
examples/reduction_longestcommonsubsequence_to_ilp.rs Example with JSON export
tests/suites/examples.rs Register the example test
docs/paper/reductions.typ Problem definition and reduction rule documentation
docs/paper/references.bib Add Maier 1978 and Blum 2021 references

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +89 to +90
strings.len() >= 2,
"LCS to ILP reduction requires at least 2 strings"
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reduction asserts strings.len() >= 2 but only uses strings[0] and strings[1], silently ignoring any additional strings. For 3+ strings, the ILP solution would be incorrect because constraints from the extra strings are not encoded. Since this reduction is scoped to 2 strings only (as stated in the issue), this should assert strings.len() == 2 to prevent silently producing wrong results.

Suggested change
strings.len() >= 2,
"LCS to ILP reduction requires at least 2 strings"
strings.len() == 2,
"LCS to ILP reduction is defined for exactly 2 strings"

Copilot uses AI. Check for mistakes.
}

crate::declare_variants! {
LongestCommonSubsequence => "2^total_length",
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The complexity string "2^total_length" is incorrect. The brute-force search space is determined by dims(), which returns vec![2; shortest_len()] — so the complexity should be 2^min_string_length (where min_string_length corresponds to the shortest string). total_length is the sum of all string lengths, which grossly overestimates the actual search space. You would need to either expose a public min_string_length() getter method for the macro to call, or rename shortest_len() to be public with an appropriate name.

Suggested change
LongestCommonSubsequence => "2^total_length",
LongestCommonSubsequence => "2^min_string_length",

Copilot uses AI. Check for mistakes.
Comment thread src/rules/mod.rs Outdated
Comment on lines 46 to 49
mod longestcommonsubsequence_ilp;
#[cfg(feature = "ilp-solver")]
mod ilp_qubo;
#[cfg(feature = "ilp-solver")]
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The longestcommonsubsequence_ilp module is placed between ilp_bool_ilp_i32 and ilp_qubo, breaking the alphabetical ordering used by the other modules in this block. It should be placed after knapsack_ilp or before maximumclique_ilp — actually, there's no knapsack_ilp, so it should be placed after ilp_qubo (since "l" > "i") and before maximumclique_ilp (since "l" < "m") to maintain alphabetical order.

Suggested change
mod longestcommonsubsequence_ilp;
#[cfg(feature = "ilp-solver")]
mod ilp_qubo;
#[cfg(feature = "ilp-solver")]
mod ilp_qubo;
#[cfg(feature = "ilp-solver")]
mod longestcommonsubsequence_ilp;
#[cfg(feature = "ilp-solver")]

Copilot uses AI. Check for mistakes.
GiggleLiu and others added 3 commits March 13, 2026 08:36
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…, fix module order

- Assert strings.len() == 2 (not >= 2) in LCS-to-ILP reduction to prevent
  silently ignoring extra strings
- Fix complexity from 2^total_length to 2^min_string_length to match actual
  brute-force search space (dims() uses shortest string)
- Fix alphabetical ordering of longestcommonsubsequence_ilp in rules/mod.rs

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@zazabap zazabap added bug Something isn't working and removed bug Something isn't working labels Mar 13, 2026
GiggleLiu and others added 2 commits March 13, 2026 14:54
Inspired by PR #170 which tested this edge case.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@GiggleLiu
Copy link
Copy Markdown
Contributor

Review Pipeline Report

Check Result
Copilot comments 3 already fixed (assert ==2, complexity string, module order)
Issue/human comments 4 issue quality checks reviewed, all concerns addressed in code
Merge with main Clean merge, no conflicts
CI green (Clippy, Test, Coverage, Codecov all pass)
Agentic test (model) passed — LCS creation, brute force, CLI all work
Agentic test (reduction) passed — ILP reduction, closed-loop verification, example all work
Edge case from PR #170 Added empty string test (inspired by PR #170)
Board review-agentic (needs manual move — auth scope missing)

Notes

🤖 Generated by review-pipeline

@GiggleLiu GiggleLiu merged commit 3689ff6 into main Mar 13, 2026
5 checks passed
@zazabap zazabap mentioned this pull request Mar 15, 2026
2 tasks
@GiggleLiu GiggleLiu deleted the issue-110-lcs-to-ilp branch April 12, 2026 00:50
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.

[Rule] LongestCommonSubsequence to ILP

3 participants