Phonons: q-only supercell diagonalization, in-place sort, LO-TO and AdjustQStar fixes - #126
Open
SorBalda wants to merge 2 commits into
Open
Phonons: q-only supercell diagonalization, in-place sort, LO-TO and AdjustQStar fixes#126SorBalda wants to merge 2 commits into
SorBalda wants to merge 2 commits into
Conversation
…djustQStar fixes Four independent changes to Phonons.py, all in the fast DiagonalizeSupercell except the last. 1. New kwarg q_only=False. When True the (3N, 3N) supercell polarization matrix is never allocated: mode selection is unchanged, but for each accepted mode only its origin (iq, band) is recorded, in two O(3N) integer arrays, so that w_q[mode_band[k], mode_iq[k]] == w_mu[k]. Callers that only need the q-space eigenmodes (the q-space TDSCHA path) then run in memory linear in the supercell size instead of quadratic. This path also skips the pre-computed phase table, which is (3N, nq) and would be the largest allocation left; the phase is recomputed one q at a time, agreeing with the table to BLAS rounding, and w_q/pols_q do not depend on it. Verified on a 27-q gold dyn: w_mu, w_q and pols_q are bitwise identical to return_qmodes=True, and the (iq, band) map is exact. 2. Sort the polarization columns in place. e_pols_sc[:, sort_mask] is fancy indexing, so the sorted copy is built while the original is still alive, doubling the peak of the routine's largest allocation. Replaced by a cycle-following permutation with a single length-3N temporary column; verified bitwise equal to the fancy-indexed result over 200 random permutations and on real dyns with heavily degenerate spectra. 3. Honour the LO-TO warning. With lo_to_split set but no effective charges the code warned "LO-TO ignored" and then ran the branch anyway. Since ForceTensor.Tensor2.Interpolate guards the whole non-analytic term behind "effective_charges is not None", that branch rebuilt the dynamical matrix DyagDinQ already has, after allocating a full supercell Tensor2 through SetupFromPhonons -> GetRealSpaceFC (measured: 132 MB at a 8x8x8 supercell, growing as N^2). Falling through to DyagDinQ gives the same frequencies and keeps the Gamma polarization gauge consistent with the other q points. The lo_to_split string validation is moved above the check so a typo still raises when the charges are missing. Applied to both copies of the routine. 4. AdjustQStar swapped the block of the last q visited by the search loop instead of the Gamma one: the loop had no break, so iq kept its final value, and the reordering was corrupted whenever Gamma was neither first nor last. It also proceeded silently when no Gamma point was present, in which case the swap zeroed q_tot[0] and fabricated a Gamma that did not exist; it now raises.
| timer.add_timer("Sort and validate", t_sort_end - t_sort_start) | ||
| return w_array, mode_iq, mode_band, w_q, pols_q | ||
|
|
||
| # Sort the supercell polarization columns IN PLACE. Fancy indexing |
Collaborator
There was a problem hiding this comment.
Here needs a better comment explaining what the following code does rather than saying what was bad about the old code
mesonepigreco
requested changes
Aug 2, 2026
mesonepigreco
left a comment
Collaborator
There was a problem hiding this comment.
Ok, just a more explicative comment in the last part of the edit (which is really incomprensible code).
Moreover, to comply with the update in the tdscha, move the version naming to 1.7.0. We need to do it anyway. (This needs to be done both in meson and in the python toml settings.
The comment described what was wrong with the fancy-indexed version and left the cycle-following loop itself unexplained. Describe the algorithm instead: why a permutation can be applied in place one cycle at a time, what the saved column and the _done mask are for, and what each step of the walk does.
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.
Phonons: q-only supercell diagonalization, in-place sort, LO-TO and AdjustQStar fixes
Part of a three-repository series that makes the q-space TDSCHA path linear in
memory instead of quadratic. This one carries the primitive the other two build
on; it is useful on its own for the two bug fixes at the end.
All four changes are in
cellconstructor/Phonons.py, and all but the last arein the fast
DiagonalizeSupercell.1. New kwarg
q_only=FalseWhen
True, the(3N, 3N)supercell polarization matrix is never allocated.Mode selection is unchanged; for each accepted mode only its origin
(iq, band)is recorded, in two
O(3N)integer arrays, so thatw_q[mode_band[k], mode_iq[k]] == w_mu[k]. Callers that only need the q-spaceeigenmodes then run in memory linear in the supercell size.
This path also skips the pre-computed phase table, which is
(3N, nq)and wouldbe the largest allocation left; the phase is recomputed one q at a time. That is
a gemv against a gemm, so the two agree to BLAS rounding, and
w_q/pols_qdonot depend on it at all.
Verified on a 27-q dyn:
w_mu,w_qandpols_qare bitwise identical toreturn_qmodes=True, and the(iq, band)map is exact. Also checked onnon-cubic grids (
[4,3,3],[4,4,3]) and before/afterAdjustQStar.2. Sort the polarization columns in place
e_pols_sc[:, sort_mask]is fancy indexing, so the sorted copy is built whilethe original is still alive, doubling the peak of the routine's largest
allocation. Replaced by a cycle-following permutation using a single length-3N
temporary column.
Verified bitwise equal to the fancy-indexed result over 200 random permutations
and on real dyns with heavily degenerate spectra (up to 1541 degenerate modes
out of 1920).
3. Honour the LO-TO warning
With
lo_to_splitset but no effective charges, the code warned"LO-TO ignored"and then ran the branch anyway. SinceForceTensor.Tensor2.Interpolateguards the whole non-analytic term behindeffective_charges is not None, that branch rebuilt the dynamical matrixDyagDinQalready has — after allocating a full supercellTensor2throughSetupFromPhonons→GetRealSpaceFC. Measured: 132 MB at an 8x8x8 supercell,growing as N².
Falling through to
DyagDinQgives the same frequencies and keeps the Gammapolarization gauge consistent with the other q points. The
lo_to_splitstringvalidation is moved above the check, so a typo still raises when the charges are
missing. Applied to both copies of the routine.
4.
AdjustQStarswapped the wrong blockThe search loop had no
break, soiqkept its final value and the swap usedthe last q visited instead of the Gamma one: the reordering was corrupted
whenever Gamma was neither first nor last. It also proceeded silently when no
Gamma point was present, in which case the swap zeroed
q_tot[0]and fabricateda Gamma that did not exist. It now raises.
Testing. No test data is added here. The claims above were checked against
gold and LaAlO3 dyns outside the repo; happy to add a small committed case if
you would like one.