fix(v1): sparse merge keeps absent cells absent instead of zero-filling (#928) - #931
Conversation
…ng (#928) CSRPayload.from_expression no longer zero-fills a NaN const when converting a dense summand; materialize applies absorb_absence so absent cells carry no terms, matching the dense v1 merge path.
Merging this PR will improve performance by 82.45%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_to_lp[expression_arithmetic-n=10] |
1,338.6 KB | 733.7 KB | +82.45% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing layer0-928 (7abdd41) with master (b71e9a9)
Footnotes
-
181 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Build cost — v1 vs legacyv1 build peak & time relative to legacy, on this commit — not a comparison against master (that is CodSpeed).
Full table (time + peak, mean)📊 Interactive plots + CSV: download the semantics-report-v1-vs-legacy artifact from this run. Report-only · not a gate · refreshed on every push · obsolete once legacy is dropped. |
Part of layer 0 in #756 (comment). Rather a small but needed fix.
Closes #928.
Note
The following content was generated by AI (Claude Code).
Root cause.
CSRPayload._from_scatterzero-filled a NaN constant for every caller. That matches the dense group kernel infrom_grouper(a NaN member counts as 0), but it is wrong infrom_expression, which converts a dense summand duringtry_csr_merge: there a NaN const means the cell is absent and must propagate, as the dense v1 merge does viaskipna=Falseandabsorb_absence.Fix.
_from_scattertakes askipnaflag:from_grouperpassesTrue,from_expressionpassesFalse, so an absent cell keeps its NaN const in the payload.materializeappliesabsorb_absence, so the dense expansion carries no terms in absent cells (the CSR rows still hold the other summand's terms).CSRConstraint.from_payloadneeded no change: it moves the const into the rhs and already drops NaN-rhs rows.Test.
test_merge_keeps_absent_cell_absentmerges a sparse group sum with a.where-masked dense one, asserts equality with the all-dense merge, and asserts the frozenCSRConstraintdrops the masked rows.Repro from the issue, after the fix