fix(translate): drop identically-zero terms to fix the correlation regression#19
Closed
oameye wants to merge 1 commit into
Closed
fix(translate): drop identically-zero terms to fix the correlation regression#19oameye wants to merge 1 commit into
oameye wants to merge 1 commit into
Conversation
…tion The time-dependent path of `translate_qo` kept terms whose coefficient was identically zero after parameter substitution. A coupling substituted to zero leaves an unfolded `conj(0.0)` factor (or `conj(conj(0.0))` when a cascade Hamiltonian's second cross-term is formed as `adjoint(X)`), which Symbolics does not fold to zero, so the term survived and the compiled closure re-evaluated it (pulse interpolation plus operator rebuild) on every ODE step. Fold `conj` of a numeric literal in each coefficient, then drop the coefficients that become zero. `conj` of a symbolic time variable is left untouched, so genuinely time-dependent complex couplings are unaffected. Effect on the correlation benchmark (single-photon cavity, g_v set to 0): allocations fall from 5.17M to 2.78M and memory from 555 MB to 373 MB, both back to the SecondQuantizedAlgebra 0.4.5 level; per-call H(t) allocation goes from 6128 B to 2080 B. Output is bit-identical (dropping identically-zero terms is exact), numeric SLH composition is unaffected (this does not touch the cascade), and translate construction time is unchanged. test_translate and test_correlations pass.
Member
Author
|
Superseded by the proper fix in SecondQuantizedAlgebra: qojulia/SecondQuantizedAlgebra.jl#193. The dead terms this PR dropped in Closing rather than patching QIO. Once SQA #193 is released, QIO just needs a |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The benchmark CI was right after all: there's a real regression in the
Correlations/two-time(and time-dependent interaction-picture) benchmark, and it's not cross-runner noise. I tracked it down totranslate_qo.The time-dependent path kept terms whose coefficient is identically zero once the parameters are substituted. When a coupling is set to zero, the substitution leaves a
conj(0.0)factor (orconj(conj(0.0))when the cascade Hamiltonian's second cross-term is built asadjoint(X)), and Symbolics doesn't fold either back to 0. So the term looked nonzero, survived into the compiled closure, and got re-evaluated (pulse interpolation + operator rebuild) on every single ODE step. Theg_v => 0coupling in the correlation benchmark hits this directly.The fix folds
conjof a numeric literal in each coefficient and drops the ones that collapse to zero.conjof a symbolic time variable is left alone, so genuinely time-dependent complex couplings are untouched.Measured on the correlation benchmark (same machine,
g_v = 0):So it's back to the old 0.4.5 allocation level (actually a hair below per-call). Output is bit-identical (dropping identically-zero terms is exact), numeric SLH composition is unchanged since this doesn't touch the cascade, and translate construction time is unchanged.
test_translate(37) andtest_correlations(4) pass locally.Worth noting: the earlier "it's just runner noise" framing on PRs #15/#18 was wrong, this was a genuine ~1.5-2x slowdown that the minimum/gcsample work correctly surfaced instead of hiding.