Skip to content

Fix priority indices under equality reduction, generated tree storage with soft constraints, and the default objective bound in branch and bound - #4

Open
baggepinnen wants to merge 1 commit into
dyadfrom
fix-review-findings
Open

baggepinnen wants to merge 1 commit into
dyadfrom
fix-review-findings

Conversation

@baggepinnen

Copy link
Copy Markdown
Collaborator

Fixes three defects of branch and bound on dyad. A codex review of JuliaComputing/daqp#2 found them in code that predates that PR.

  1. Priority list with equality reduction (introduced in ef72ae7). daqp_set_branch_priority stores indices of the original problem. When an equality elimination is installed, daqp_get_branch_id used them as indices of the reduced problem, which reads beyond work->sense; AddressSanitizer reports a heap-buffer-overflow. The indices are now mapped through eq->keep, which is in ascending order of the original index, so the lookup is a binary search; eliminated constraints are skipped. Code generation of a reduced problem writes the mapped list.
  2. Generated tree storage (since 9393ffb). Generated code declared tree_WS[(n+1)(nb+1)], while setup_daqp_bnb allocates (n+ns+1)(nb+1), because soft constraints can be part of a stored working set. With ten soft constraints the generated array has 6 entries and the root working set writes 10. The generated size now includes the soft constraints of the generated problem.
  3. Default objective bound.
    • DAQP_INF = 1e30 is finite. The bound was shifted by the offset c = 0.5||v||^2 - fp only when rel_subopt, fval_bound or lb was set. With the default bound it was therefore compared with the internal objective J + c instead of J.
    • Example: 0.5x^2 + 2e15 x with x binary is reported infeasible instead of x = 0 with objective 0.
    • The offset is now always added (O(n) per branch-and-bound solve).
    • Consequence: problems whose objective J itself exceeds 1e30 are still pruned, but problems with a large c and a moderate J are no longer. This is the behaviour the parallel solve in JuliaComputing/daqp#1 needs, since it shares the bound on J between threads.

Not changed: the continuous solver daqp_ldp (no binaries) still compares 2*fval_bound with the internal objective without c.

Tests

  • New interfaces/daqp-eigen/tests/32_bnb_regressions.cpp, one case per defect.
  • On dyad without this change: cases 2 and 3 fail, and case 1 aborts under AddressSanitizer.
  • With the change: all pass, also under AddressSanitizer. ctest 32/32, Julia suite 1479/1479.

Host: demeter2, session session_01JGy8Ya4ZvmiQUA33aivGGJ, working directory /tmp/daqpfork

🤖 Generated with Claude Code

- A priority list holds indices of the original problem, but branching read them
  as indices of the problem in the workspace, which differs if an equality
  elimination is installed (read beyond the senses of the reduced problem). The
  indices are now mapped through eq->keep, and code generation writes the mapped
  list for a reduced problem.
- Generated code allocated (n+1)(nb+1) entries for the working sets of the tree,
  while the library allocates (n+ns+1)(nb+1) since soft constraints can be part of
  a working set (write beyond the array). The generated size now includes them.
- The default bound DAQP_INF on the objective is finite and was compared with the
  internal objective 0.5||u||^2 = J + c without the offset c, so nodes of a problem
  with a large ||v|| were pruned and the problem reported infeasible. The offset is
  now always added, so that the bound applies to J.

Regression test 32_bnb_regressions (fails on the previous code; the first case
is detected by AddressSanitizer).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@baggepinnen

Copy link
Copy Markdown
Collaborator Author

Consequence of item 3 for data whose linear term is of the order of DAQP_INF (e.g. the LinearMPC example invpend_contact, with a linear term of about 1e30): c is then so large that DAQP_INF + c equals c in floating point, and the objective J cannot be resolved. Before this change such a problem was reported infeasible at the root (since J + c > DAQP_INF); now the search proceeds and, on invpend_contact with presolve, ends at the time limit with an incumbent of objective 1.8e44. Without presolve it is still reported infeasible. The data of that example are not meaningful (a linear term equal to the value that represents infinity), so I did not add special handling; a check for |c| beyond the resolution of DAQP_INF would be possible if the previous behaviour is preferred.

baggepinnen added a commit that referenced this pull request Nov 9, 2026
The bound of branch and bound is now always shifted by the offset c, so the
parallel solve shares and applies the same bound as the single solve. The Julia
regression test of the default bound now requires that the single and the
parallel solve agree, since the objective J of that problem is below the bound.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
baggepinnen added a commit that referenced this pull request Nov 9, 2026
child_order is not compared by daqp_parallel_compatible, so the threads of a
parallel solve may use different child orders.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
baggepinnen added a commit that referenced this pull request Nov 9, 2026
node_selection and node_pool_size are not compared by daqp_parallel_compatible,
so the threads of a parallel solve may use different node selections.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.

1 participant