Reorganization energy: persist results, compare geometries, compare λ - #44
Merged
Conversation
Student report: reorg results reloaded from History came back without the numbers the calculation exists to produce. It presented as a display bug and was not one. save_result wrote a fixed schema of scalar fields with no `channels` key, so the λ breakdown was never persisted. The live card rendered from an object still in memory; the History card had nothing behind it. Repairing the formatter could not have worked, and would have been the obvious first thing to try. ── The structural fix ────────────────────────────────────────────────── Both cards now render from the SAME plain-dict payload. The live path converts its result objects into the shape the saved file holds, so "renders live" implies "renders after reload" by construction rather than by two implementations happening to agree. That drift is the bug, so removing the possibility of drift is the fix. λ is a first-class schema field, not an entry in the generic `extras` bag: it is the headline number of this calculation type, and burying it would leave it undiscoverable and unqueryable. ── Geometries, retained and used ─────────────────────────────────────── The ion-optimized geometry was computed and thrown away — it existed only as a local. Without it the four energies cannot be interpreted after the fact: λ measures how far the molecule relaxed on becoming an ion, and there was nothing to compare R_neutral against. It is now kept and persisted, which makes RMSD and largest-atom-shift a few lines of numpy. Those are computed at DISPLAY time from the stored coordinates rather than persisted as separate numbers that could drift out of step with the geometry they describe. The neutral geometry rides inside the channel payload rather than being read from the top level, which stores atom_symbols but not coordinates — so a history card had no reliable way to reach R_neutral at all. Also corrected a misconception in the original request, with the user's agreement: the Marcus scheme is four energies on TWO geometries per channel (three across both channels), not four geometries. A four-way comparison would show each geometry twice and imply all four differ. ── Old results ───────────────────────────────────────────────────────── They cannot be recovered — λ is two optimizations and four SCF energies — so the card says so and names the remedy instead of rendering an empty section that reads like a rendering failure. Detected by ABSENCE of the payload, not by version or timestamp: a result re-saved or imported from elsewhere would defeat a version cutoff. ── Tests ─────────────────────────────────────────────────────────────── 15 new, exercising the real save → load → render path rather than either formatter — inspecting a formatter is exactly what would have missed this, since the live one was always correct. The load-bearing test asserts both cards render the same channel content. Remaining in M-REORG: the geometry stepper, the overlay view, and wiring the calc type into the Analysis tab (it has no _PANEL_REGISTRY entry at all, so that tab is currently blank for these runs). The data foundation they need is what this commit lands. Full suite: 2131 passed, 23 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Builds on the persistence work: with the geometries retained, the four energies can finally be inspected rather than only summarised. ── The distinct geometries, not four ─────────────────────────────────── The Marcus scheme evaluates four energies on TWO geometries per channel — E_ion(R_neutral) shares its geometry with E_neutral(R_neutral), likewise for R_ion — so a hole+electron run has three distinct structures, not six and not the four the name suggests. The stepper shows each once and labels it with which energies were evaluated on it, so the picture stays connected to λ. Not animated: λ is a comparison between states, not a trajectory through them, and looping would imply a path never computed. Built on _frame_stepper_controls rather than a second stepper, so camera handling, offline loading and control styling match the trajectory viewer. ── The overlay, rebuilt after seeing it ──────────────────────────────── First version drew two solid ball-and-stick models in equal weight. For structures that nearly coincide — which is most of them, since λ is usually a small relaxation — that is an unreadable blob. Reported immediately, and correctly. The hierarchy now matches the question: reference is a thin grey wireframe (context), the relaxed geometry a thin coloured one, and ARROWS from each atom's reference position to its relaxed one carry the signal. An arrow has direction and length, which is what a displacement is; two overlapping solids have neither. Atoms that did not move get no arrow — a zero-length arrow is a dot and reads as noise. Added an arrow-scale control (x1/x3/x5/x10) because λ relaxations are often hundredths of an Angstrom: real, invisible at 1:1. It scales the ARROWS ONLY, never the structures, so every atom stays at its computed position and the legend states the factor. Nothing displayed is fictional. No Kabsch alignment either — both geometries came from optimizations seeded identically, so the displacement is physical and superimposing would rotate away part of what λ measures. ── The Analysis tab existed but was empty ────────────────────────────── reorganization_energy had NO _PANEL_REGISTRY entry, so that tab populated nothing for these runs. Now registered as Energies -> Geometries -> Isosurface, which satisfies both constraints: Energies precedes Isosurface (it loads the orbital state Isosurface checks) and Geometries is the first auto_select so it opens by default. ── A miss my own tests could not catch ───────────────────────────────── The accordion was built, registered in _PANEL_META, and never added to the Analysis VBox's children — so it could never render. Every unit test passed because each checked one layer; nothing checked the layers were connected. Only a screenshot caught it. The new test is therefore structural rather than about this panel: it walks every _PANEL_META entry and asserts the accordion is a child of the tab. The next panel added is covered without anyone remembering to. That is the third "registered but not wired" gap this session, so the guard belongs at the seam rather than on the instance. Full suite: 2147 passed, 23 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
── λ comparison ──────────────────────────────────────────────────────── Selecting several reorganization-energy results in the Compare tab now yields a λ table beneath the general one, with a column per channel. This is the workflow the calculation type exists for — screening candidates by how much they reorganize — and a single λ is hard to judge without others beside it. A SEPARATE table rather than extra columns on the general comparison: λ is per-channel, so folding it into a one-row-per-result grid would mean either duplicating rows or inventing a combined number with no physical meaning. Results predating λ persistence are listed with "re-run to compare" rather than skipped — silently omitting them would look like they were never selected. The table also states that lower λ is favourable for charge transport, since a bare number invites the wrong reading and this is one of the few quantities where the direction is unambiguous. ── Sticky background ─────────────────────────────────────────────────── The geometry viewers were missing from rerender_3d_scenes_for_theme. Same bake-in as every other py3Dmol viewer — bgcolor is painted into the scene at render time — and cheap to redraw here, since the geometries are a few dozen coordinates already in memory. ── A test of yours caught me ─────────────────────────────────────────── test_retired_border_greys_are_gone_from_in_app_chrome, written during THEME.5, flagged that the new comparison table used #cbd5e1 and #e2e8f0. Those are exactly the greys that vanish under dark mode's whole-page inversion. Now theme.BORDER. Without that test this would have shipped as "the λ table has no visible rules in dark mode" and surfaced weeks later. Full suite: 2153 passed, 23 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
NCCU-Schultz-Lab
added a commit
that referenced
this pull request
Aug 5, 2026
The first real H200 allocation came back 7/7 green with a clean CPU->GPU crossover, so the pairing that worked is now written down where the next build can compare against it: driver 580.126.20, compute capability 9.0, partition gpu, cuda12x wheels. Notably CuPy reports driver API 13000 against runtime 12090 — exactly the backward-compatible combination that made cuda12x the right call over cuda13x, which would have hard-failed on the 570-series driver the hardware notes originally listed. The diagnostic notebook now reports the CPU AFFINITY MASK, not just the core count. Slurm can grant 6 cores two ways: a cgroup quota (the mask still shows all 192, so OpenMP spawns 192 threads and thrashes them) or a cpuset (the mask shrinks and an unset OMP_NUM_THREADS is harmless). Same request, same cpu_count, opposite consequences — so the number the notebook was printing could not distinguish them. It now states plainly when CPU timings cannot be trusted, and gives the one-line fix. On this run the mask was 6, so the measured crossover stands. It is recorded with its CPU allocation, because a speedup without its denominator is the first thing an audience challenges — and the node has ~12 cores per GPU, so 6 is less than a proportional share. Version 0.6.1: this release is the reorganization-energy work merged in #44 plus these docs. Patch rather than minor — nothing here changes an API or removes a capability. Full suite: 2153 passed, 23 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Fixes a student-reported bug and closes most of M-REORG.
The bug
Reorganization-energy results reloaded from History came back missing the numbers the calculation exists to produce.
It presented as a display bug and was not one.
save_resultwrote a fixed schema of scalar fields with nochannelskey, so the λ breakdown was never persisted — the live card rendered from an object still in memory, and the History card had nothing behind it. Repairing the formatter could not have worked, and would have been the obvious first thing to try.Both cards now render from the same plain-dict payload, so "renders live" implies "renders after reload" by construction rather than by two implementations happening to agree. That drift was the bug, so removing the possibility of drift is the fix.
What a user gets
Reviewer notes
The overlay's first version was unreadable. Two solid ball-and-stick models in equal weight, for structures that nearly coincide — which is most of them, since λ is usually a small relaxation. Rebuilt so the hierarchy matches the question: thin grey reference, thin coloured relaxed geometry, and arrows carrying the signal. An arrow has direction and length, which is what a displacement is; two overlapping solids have neither.
The arrow-scale control (×1–×10) scales arrows only, never structures — atoms always sit at computed positions and the legend states the factor, so nothing displayed is fictional. No Kabsch alignment either: both geometries came from optimizations seeded identically, so superimposing would rotate away part of what λ measures.
The Analysis tab was empty for these runs —
reorganization_energyhad no_PANEL_REGISTRYentry at all. NowEnergies → Geometries → Isosurface, satisfying both constraints: Energies precedes Isosurface (it loads the orbital state Isosurface checks) and Geometries is the firstauto_select.A miss the tests couldn't catch. The accordion was built, registered, and never added to the Analysis VBox's children — so it could never render. Every unit test passed because each checked one layer; only a screenshot caught it. The new test is therefore structural: it walks every
_PANEL_METAentry and asserts the accordion is a child of the tab, so the next panel is covered without anyone remembering to. That was the third "registered but not wired" gap in this session, so the guard belongs at the seam.Still open
The internal optimization trajectories are still discarded, so the Trajectory panel doesn't light up for these runs — small now that the geometries are retained. XYZ and PNG export moved to a new milestone.
37 reorg tests. Full suite: 2153 passed, 23 skipped.
Generated with Claude Code