Skip to content

Fix-dist fit upper bound - #133

Open
ishikaghosh2201 wants to merge 3 commits into
masterfrom
fix/dist-fit-upper-bound
Open

ishikaghosh2201 wants to merge 3 commits into
masterfrom
fix/dist-fit-upper-bound

Conversation

@ishikaghosh2201

@ishikaghosh2201 ishikaghosh2201 commented Oct 3, 2026 •

Copy link
Copy Markdown
Collaborator

Description

solve_ilp_dist() (in cereeberus/distance/ilp.py) built its constraints by iterating over the union of both graphs' function values. It assumed every level had boundary, assignment and distance blocks for the starting graph. When the two graphs have different ranges (for example G = F.smoothing(n), which extends [0..3] to [-1..4]), the solver looked up a block that doesn't exist and raised KeyError.

Changes:

  • ilp.py / solve_ilp_dist()
    • Skip levels that aren't in the starting graph's own range: if block not in myAssgn.all_func_vals(map=starting_map): continue.
    • Skip the last up/down diagram at the starting graph's own top level (all_func_vals(map=starting_map)[-1]), not the top of the combined range.
    • Both changes match how the feasibility solver solve_ilp() already handles this.
    • Audit of the rest of the constraint loop: the remaining lookups either use the thickened graphs, which cover the whole combined range, or use the starting graph at levels the new guard has already checked. The block + 1 lookup is safe because the top level is skipped. No other changes were needed.
  • interleave.py / Interleave.dist_fit()
    • Track best_bound = n + loss and tighten the binary search's upper end with high = min(high, best_bound - 1) after each nonzero-loss trial, so the search narrows faster.
  • Tests: added TestDistOptimizeUnequalRange to tests/test_interleaving.py.
  • Ran black on the changed files.

Motivation and Context

Fixes #132
Minimal reproduction:

from cereeberus import Assignment
from cereeberus.data.ex_mappergraphs import line

F = line(0, 3)
G = F.smoothing(1)
Assignment(F, G, n=1).dist_optimize()  # KeyError: -1

The feasibility path finds a zero-loss 1-interleaving, so the expected result is 0. Interleave(F, G).dist_fit() failed the same way.

How has this been tested?

  • New regression tests in TestDistOptimizeUnequalRange:
    • test_graph_vs_smoothing: line(0,3) against its 1-smoothing, in both orders. Checks that optimize() succeeds with loss() == 0, that dist_optimize() == 0, and that dist_fit() == 1.
    • test_offset_ranges: line(0,3) against line(1,5) with n=2. Checks that dist_optimize() == 0.
    • Both tests fail on master with KeyError and pass with this change.
  • Checked by hand that dist_optimize() agrees with optimize()/loss(), and dist_fit() with fit(), on these pairs:
    • the line and its smoothing, in both orders
    • line(0,3) with line(1,5)
    • line(0,3) with line(2,4)
    • simple_loops with its smoothing
  • All tests in tests/test_interleaving.py pass, and the rest of the suite is unaffected.
  • Note: PuLP 4.0 removed LpVariable.dicts, which breaks both ILP solvers, so testing used pulp<4. This should probably be pinned in a separate PR.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have incremented the version number in the pyproject.toml file if a new version needs to be pushed to pypi. Note that if the number isn't incremented, the package will not be pushed to pypi, which is useful if this PR is only for updating documentation.
  • My code follows the code style of this project and I have run make format to clean up the code with black.
  • My change requires a change to the documentation. I have updated the documentation as necessary and compiled locally to ensure it is clean.
  • I have added tests to cover my changes, and all new and existing tests passed (run make tests).

@ishikaghosh2201
ishikaghosh2201 requested review from lizliz and a balanced review from Copilot October 3, 2026 04:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

This branch has not been deployed

No deployments
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.

Possible error in dist_fit

2 participants