Conversation
…Replace linear degree-bucket search with O(1) swap-with-last removal using col_pos/row_pos position arrays, and eliminate O(row_degree) pre-traversal in schur_complement via a persistent last_in_row[] array
rg20
added
improvement
|
/ok to test 5f377a2 |
📝 WalkthroughWalkthroughEnhanced dual simplex optimization by introducing sparse delta_y computation in crossover operations and adding persistent position-tracking state (col_pos, row_pos, last_in_row) to replace linear searches with constant-time operations in LU factorization routines. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cpp/src/dual_simplex/crossover.cpp (1)
418-436: Consider hoistingdelta_expandedallocation outside the while loop.Currently
delta_expandedis allocated as a newstd::vector<f_t>(n, 0.)on every iteration of thewhile (superbasic_list.size() > 0)loop (starting at line 391). For large problems with many superbasic variables, this results in repeated O(n) allocations and deallocations.Moving the allocation before the loop and using
std::fillto reset would avoid this overhead.♻️ Suggested refactor
+ std::vector<f_t> delta_expanded(n); while (superbasic_list.size() > 0) { // ... existing code ... // delta_zN = -N^T delta_y std::vector<f_t> delta_zN(n - m); - std::vector<f_t> delta_expanded(n, 0.); + std::fill(delta_expanded.begin(), delta_expanded.end(), 0.); // Iterate directly over sparse delta_y instead of checking zeros🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/dual_simplex/crossover.cpp` around lines 418 - 436, Hoist the allocation of delta_expanded (and optionally delta_zN) out of the while (superbasic_list.size() > 0) loop: create std::vector<f_t> delta_expanded(n) once before the loop and inside each iteration reset its contents with std::fill(delta_expanded.begin(), delta_expanded.end(), 0.0) (and similarly reset delta_zN or ensure it is resized once and overwritten). Update the loop body that accumulates into delta_expanded and the subsequent assignment to delta_zN[k] = -delta_expanded[nonbasic_list[k]] to rely on the reused buffers; if n can change, ensure you call delta_expanded.resize(n) before the fill.cpp/src/dual_simplex/right_looking_lu.cpp (1)
649-661: Extract the bucket-position bootstrap into one helper.The
col_pos/row_posinitialization is now duplicated in both factorization entry points, with only the bucket bounds differing. This is subtle invariant code; a small helper next toinitialize_degree_data()would reduce drift the next time the bucket bookkeeping changes.Also applies to: 1049-1062
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/src/dual_simplex/right_looking_lu.cpp`:
- Around line 405-409: work_estimate is using Cdegree[pivot_j] and
Rdegree[pivot_i] after those entries have been set to -1 by
update_Cdegree_and_col_count() / update_Rdegree_and_row_count(), causing
work_estimate to decrease incorrectly; fix by using the actual degree before it
is clobbered — either capture a local variable like int deg_j = Cdegree[pivot_j]
and/or deg_i = Rdegree[pivot_i] before calling the update functions, or compute
the degree from the corresponding count arrays (e.g. col_count[pivot_j] /
row_count[pivot_i]) or by traversing first_in_col/first_in_row if counts are not
available — then use deg_j/deg_i when updating work_estimate instead of reading
Cdegree/Rdegree after they were set to -1.
---
Nitpick comments:
In `@cpp/src/dual_simplex/crossover.cpp`:
- Around line 418-436: Hoist the allocation of delta_expanded (and optionally
delta_zN) out of the while (superbasic_list.size() > 0) loop: create
std::vector<f_t> delta_expanded(n) once before the loop and inside each
iteration reset its contents with std::fill(delta_expanded.begin(),
delta_expanded.end(), 0.0) (and similarly reset delta_zN or ensure it is resized
once and overwritten). Update the loop body that accumulates into delta_expanded
and the subsequent assignment to delta_zN[k] = -delta_expanded[nonbasic_list[k]]
to rely on the reused buffers; if n can change, ensure you call
delta_expanded.resize(n) before the fill.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6a904205-beca-40f2-bce0-a3dcdb15be1e
📒 Files selected for processing (2)
cpp/src/dual_simplex/crossover.cppcpp/src/dual_simplex/right_looking_lu.cpp
| // Initialize row_last_workspace from last_in_row (O(1) per row, no full row traversal) | ||
| for (i_t p1 = first_in_col[pivot_j]; p1 != kNone; p1 = elements[p1].next_in_column) { | ||
| element_t<i_t, f_t>* e = &elements[p1]; | ||
| const i_t i = e->i; | ||
| i_t row_last = kNone; | ||
| for (i_t p3 = first_in_row[i]; p3 != kNone; p3 = elements[p3].next_in_row) { | ||
| row_last = p3; | ||
| } | ||
| work_estimate += 2 * Rdegree[i]; | ||
| row_last_workspace[i] = row_last; | ||
| row_last_workspace[elements[p1].i] = last_in_row[elements[p1].i]; | ||
| } | ||
| work_estimate += 4 * Cdegree[pivot_j]; |
There was a problem hiding this comment.
Fix work_estimate accounting after pivot elimination.
Line 409, Line 499, and Line 509 still read Cdegree[pivot_j] / Rdegree[pivot_i], but those were already set to -1 by update_Cdegree_and_col_count() and update_Rdegree_and_row_count() before this helper is called. That makes work_estimate decrease on every pivot instead of approximating the actual loop cost.
Also applies to: 499-509
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/dual_simplex/right_looking_lu.cpp` around lines 405 - 409,
work_estimate is using Cdegree[pivot_j] and Rdegree[pivot_i] after those entries
have been set to -1 by update_Cdegree_and_col_count() /
update_Rdegree_and_row_count(), causing work_estimate to decrease incorrectly;
fix by using the actual degree before it is clobbered — either capture a local
variable like int deg_j = Cdegree[pivot_j] and/or deg_i = Rdegree[pivot_i]
before calling the update functions, or compute the degree from the
corresponding count arrays (e.g. col_count[pivot_j] / row_count[pivot_i]) or by
traversing first_in_col/first_in_row if counts are not available — then use
deg_j/deg_i when updating work_estimate instead of reading Cdegree/Rdegree after
they were set to -1.
| elements[nz].x = A.x[p]; | ||
| elements[nz].next_in_column = kNone; | ||
| if (p > col_start) { elements[nz - 1].next_in_column = nz; } | ||
| elements[nz].next_in_row = kNone; // set the current next in row to None (since we don't know |
There was a problem hiding this comment.
Would you mind restoring the comments here. Those are helpful
| } | ||
| work_estimate += 2 * Rdegree[i]; | ||
| row_last_workspace[i] = row_last; | ||
| row_last_workspace[elements[p1].i] = last_in_row[elements[p1].i]; |
There was a problem hiding this comment.
Nit. I think this would read clearer if we did
const i_t i = elements[p1].i;
row_last_workspace[i] = last_in_row[i];
| initialize_degree_data(A, column_list, Cdegree, Rdegree, col_count, row_count, work_estimate); | ||
|
|
||
| // Position arrays for O(1) degree-bucket removal | ||
| std::vector<i_t> col_pos(n); |
There was a problem hiding this comment.
We should document what col_pos stores. As we do with other variables. I think it is
std::vector<i_t> col_pos(n); // if Cdegree[j] = nz, than j is in col_count[nz][col_pos[j]]
| col_pos[col_count[d][pos]] = pos; | ||
| } | ||
| } | ||
| std::vector<i_t> row_pos(n); |
There was a problem hiding this comment.
Could we add the comment
std::vector<i_t> row_pos(n); // If Rdegree[i] = nz, than i is in row_count[nz][row_pos[i]]
| std::vector<i_t>& Cdegree, | ||
| std::vector<std::vector<i_t>>& row_count, | ||
| std::vector<std::vector<i_t>>& col_count, | ||
| std::vector<i_t>& last_in_row, |
There was a problem hiding this comment.
What's the difference between row_last_workspace and last_in_row?
2 participants
Description
This PR includes two performance improvements.
Issue
Checklist