From 2fbfc21ce18700d6d7e155623443a31bc04255e8 Mon Sep 17 00:00:00 2001 From: QuantUI Bot Date: Mon, 7 Sep 2026 22:56:54 +0000 Subject: [PATCH 1/6] fix(slurm): support solvated Geometry Opt in the batch worker (audit additional concerns) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Worker's _SOLVENT_SUPPORTED_CALC_TYPES omitted "geometry_opt" while app_runflow.py's UI enables (and relabels) the solvent checkbox for Geometry Opt, documenting a real "gas-phase optimization, solvated final single point" approximation. A user who checked the box and submitted to SLURM got a hard UNSUPPORTED_CAPABILITY rejection of the whole job, even though the identical settings succeed interactively. _run_geometry_opt now mirrors app.py's interactive _run_required_final_single_point: after the gas-phase optimize_geometry call, a required solvated single point runs on the final geometry via session_calc.run_in_session, its energy replaces the optimizer's last-step energy, and its convergence is folded into the overall result — an unconverged required single point fails the job rather than silently reporting the gas-phase result. "geometry_opt" is now in _SOLVENT_SUPPORTED_CALC_TYPES alongside "single_point" and "reorganization_energy". Updated the existing parametrized rejection test to drop "geometry_opt" (it's supported now) and added tests covering the new success path and the "final single point must converge" failure path. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01E7tooXTTNTfemGJstUib8F --- quantui/backends/worker.py | 79 ++++++++++++++++++++---- tests/test_backends_worker.py | 109 +++++++++++++++++++++++++++++++--- 2 files changed, 170 insertions(+), 18 deletions(-) diff --git a/quantui/backends/worker.py b/quantui/backends/worker.py index e59aca2..239826b 100644 --- a/quantui/backends/worker.py +++ b/quantui/backends/worker.py @@ -49,16 +49,23 @@ _SUPPORTED_CALC_TYPES = frozenset(CALC_TYPES) -# AUDIT F11 — the science APIs behind these calc types (run_freq_calc, -# run_tddft_calc, run_nmr_calc, run_pes_scan, optimize_geometry) don't -# accept a solvent argument at all, so request.solvent used to be silently -# dropped rather than either applied or rejected. Only these two runners -# actually thread request.solvent through to a PCM-capable API -# (session_calc.run_in_session / reorganization_energy.run_reorganization_ -# energy, both real gas+PCM or PCM-single-point implementations — see each -# runner below and reorganization_energy's own docstring for the -# gas-phase-optimization + PCM-single-point approximation it documents). -_SOLVENT_SUPPORTED_CALC_TYPES = frozenset({"single_point", "reorganization_energy"}) +# AUDIT F11 (additional concern, code review) — the science APIs behind +# most of these calc types (run_freq_calc, run_tddft_calc, run_nmr_calc, +# run_pes_scan, optimize_geometry) don't accept a solvent argument at all, +# so request.solvent used to be silently dropped rather than either applied +# or rejected. These three runners actually thread request.solvent through +# to a PCM-capable API: session_calc.run_in_session for "single_point"; +# reorganization_energy.run_reorganization_energy for +# "reorganization_energy" (its own docstring documents the gas-phase- +# optimization + PCM-single-point approximation); and _run_geometry_opt for +# "geometry_opt", which mirrors the interactive app's approximation — +# optimize gas-phase, then run a required solvated single point on the +# final geometry (see app.py's _run_required_final_single_point) — so the +# app_runflow.py UI, which enables the solvent checkbox for these same +# three calc types, is never lying about what a submitted job will do. +_SOLVENT_SUPPORTED_CALC_TYPES = frozenset( + {"single_point", "geometry_opt", "reorganization_energy"} +) def _write_progress( @@ -358,7 +365,7 @@ def _run_geometry_opt( "attempt's checkpoint.", ) - return optimize_geometry( + result = optimize_geometry( molecule=molecule, method=request.method, basis=request.basis, @@ -371,6 +378,56 @@ def _run_geometry_opt( resume=resumable, ) + # AUDIT F11 (additional concern) — mirror app.py's interactive + # "Geometry Opt" + solvent handling: optimize_geometry has no solvent + # argument, so a solvated result here means gas-phase optimization + # followed by a required PCM single point on the final geometry, whose + # energy/convergence replace the optimizer's last-step values. Without + # this, request.solvent for geometry_opt would either be rejected + # outright (AUDIT F11) or, if permitted, silently ignored. + if request.solvent: + _write_progress( + staging_dir, "running", "Running required solvated single point", 90.0 + ) + _append_log( + staging_dir, + "\n-- Required single-point (after geometry optimisation) " + "on optimized geometry --------------------------------", + ) + from quantui.session_calc import run_in_session + + sp_result = run_in_session( + molecule=result.molecule, + method=request.method, + basis=request.basis, + progress_stream=log_stream, + solvent=request.solvent, + scf_rescue=scf_rescue, + ) + if not bool(getattr(sp_result, "converged", False)): + raise RuntimeError( + "Required post-optimization single-point did not converge." + ) + _append_log( + staging_dir, + "Required single-point converged on optimized geometry.", + ) + # ``energy_hartree`` is a read-only property derived from + # ``energies_hartree[-1]`` (there is no ``homo_lumo_gap_ev`` field on + # ``OptimizationResult`` either) — updating the last trajectory + # energy is how app.py's interactive path folds the solvated result + # in too, so both surfaces agree on what "the" final energy is. + sp_energy = getattr(sp_result, "energy_hartree", None) + if ( + isinstance(getattr(result, "energies_hartree", None), list) + and result.energies_hartree + and isinstance(sp_energy, (int, float)) + ): + result.energies_hartree[-1] = float(sp_energy) + result.converged = bool(result.converged) and bool(sp_result.converged) + + return result + def _run_frequency( request: CalculationRequest, staging_dir: Path, log_stream diff --git a/tests/test_backends_worker.py b/tests/test_backends_worker.py index 1ac2576..24c62f3 100644 --- a/tests/test_backends_worker.py +++ b/tests/test_backends_worker.py @@ -50,14 +50,14 @@ def test_unsupported_calc_type_returns_error(self, staging): assert outcome.status == "error" assert outcome.error["code"] == "UNSUPPORTED_CAPABILITY" - @pytest.mark.parametrize( - "calc_type", ["geometry_opt", "frequency", "tddft", "nmr", "pes_scan"] - ) + @pytest.mark.parametrize("calc_type", ["frequency", "tddft", "nmr", "pes_scan"]) def test_solvent_on_unsupported_calc_type_returns_error(self, staging, calc_type): - """AUDIT F11 — run_freq_calc/run_tddft_calc/run_nmr_calc/run_pes_scan/ - optimize_geometry don't accept a solvent argument at all; a - solvent set for one of these calc_types must fail the request - rather than silently run gas-phase. + """AUDIT F11 — run_freq_calc/run_tddft_calc/run_nmr_calc/run_pes_scan + don't accept a solvent argument at all; a solvent set for one of + these calc_types must fail the request rather than silently run + gas-phase. ("geometry_opt" is covered separately — see + test_solvent_on_geometry_opt_runs_required_final_single_point below + (code review) — it now has a real, documented approximation.) """ data = json.loads((staging / "request.json").read_text()) data["calc_type"] = calc_type @@ -160,6 +160,101 @@ def test_geometry_opt_success(self, mock_opt, staging): assert payload["calc_type"] == "geometry_opt" assert (staging / "trajectory.json").exists() + @patch("quantui.session_calc.run_in_session") + @patch("quantui.optimizer.optimize_geometry") + def test_solvent_on_geometry_opt_runs_required_final_single_point( + self, mock_opt, mock_run, staging + ): + """Code review (audit follow-up) — app_runflow.py's UI enables the + solvent checkbox for Geometry Opt (with a label documenting the + gas-phase-optimization + solvated-final-single-point + approximation), so a SLURM submission with the same settings must + actually apply it rather than hard-rejecting the whole job.""" + from quantui.molecule import Molecule + + mol = Molecule( + atoms=["H", "H"], + coordinates=[[0, 0, 0], [0, 0, 0.74]], + charge=0, + multiplicity=1, + ) + mock_opt.return_value = SimpleNamespace( + molecule=mol, + trajectory=[mol], + energies_hartree=[-1.10], + converged=True, + n_steps=3, + method="RHF", + basis="STO-3G", + formula="H2", + ) + mock_run.return_value = SimpleNamespace( + energy_hartree=-1.12, + homo_lumo_gap_ev=10.0, + converged=True, + n_iterations=5, + method="RHF", + basis="STO-3G", + formula="H2", + ) + data = json.loads((staging / "request.json").read_text()) + data["calc_type"] = "geometry_opt" + data["solvent"] = "water" + data["options"] = {"fmax": 0.05, "max_steps": 50} + (staging / "request.json").write_text(json.dumps(data)) + + outcome = run_worker_request(staging / "request.json") + assert outcome.status == "success" + assert mock_run.call_args.kwargs["solvent"] == "water" + # The solvated single-point energy replaces the gas-phase + # optimizer's last-step energy, mirroring app.py's interactive + # _run_required_final_single_point handling. + payload = json.loads((staging / "result.json").read_text()) + assert payload["energy_hartree"] == -1.12 + + @patch("quantui.session_calc.run_in_session") + @patch("quantui.optimizer.optimize_geometry") + def test_solvent_on_geometry_opt_final_single_point_must_converge( + self, mock_opt, mock_run, staging + ): + """An unconverged required solvated single point must fail the job + rather than silently reporting the gas-phase optimizer's result.""" + from quantui.molecule import Molecule + + mol = Molecule( + atoms=["H", "H"], + coordinates=[[0, 0, 0], [0, 0, 0.74]], + charge=0, + multiplicity=1, + ) + mock_opt.return_value = SimpleNamespace( + molecule=mol, + trajectory=[mol], + energies_hartree=[-1.10], + converged=True, + n_steps=3, + method="RHF", + basis="STO-3G", + formula="H2", + ) + mock_run.return_value = SimpleNamespace( + energy_hartree=-1.12, + homo_lumo_gap_ev=10.0, + converged=False, + n_iterations=5, + method="RHF", + basis="STO-3G", + formula="H2", + ) + data = json.loads((staging / "request.json").read_text()) + data["calc_type"] = "geometry_opt" + data["solvent"] = "water" + data["options"] = {"fmax": 0.05, "max_steps": 50} + (staging / "request.json").write_text(json.dumps(data)) + + outcome = run_worker_request(staging / "request.json") + assert outcome.status == "error" + @patch("quantui.freq_calc.run_freq_calc") def test_frequency_success(self, mock_freq, staging): mock_freq.return_value = SimpleNamespace( From 84dbba1c5b5cd4610982f84b841ee1f80ee66d4c Mon Sep 17 00:00:00 2001 From: QuantUI Bot Date: Mon, 7 Sep 2026 22:57:03 +0000 Subject: [PATCH 2/6] fix(tddft): stop requiring every TD root to converge, and don't punish missing per-root data (audit additional concerns) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AUDIT F08 made overall `converged` require ALL requested TD roots to converge, but a Davidson solve for more than a few states routinely leaves the higher/harder roots short of the default iteration budget while the lower, physically relevant ones are fine — so a completely normal multi-state UV-Vis run was always flagged "treat with caution". Separately, `td_converged is not None and all(td_converged)` forces converged=False whenever the installed PySCF doesn't expose `td.converged` at all, since `td_converged is not None` is then always False — flagging every TD-DFT/TDHF result on such a build regardless of how well it actually converged. Relaxed the rule to: SCF converged AND the TD solve produced states AND (no per-root info available, OR at least one requested root converged). This still catches the original F08 bug (every root came back unconverged), no longer penalizes a build with no td.converged attribute (treated as "unknown", not "unconverged"), and n_converged_states still reports the exact per-root tally for anyone who wants it. Added tests for partial-convergence (deterministically reproduced by patching TDHF.kernel to overwrite td.converged after a real, normal solve, since Davidson's actual partial-convergence behavior isn't reliably reproducible across PySCF versions) and for a PySCF build without a td.converged attribute. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01E7tooXTTNTfemGJstUib8F --- quantui/tddft_calc.py | 50 ++++++++++++++++++------- tests/test_tddft_calc.py | 79 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 13 deletions(-) diff --git a/quantui/tddft_calc.py b/quantui/tddft_calc.py index c0632a1..45196e4 100644 --- a/quantui/tddft_calc.py +++ b/quantui/tddft_calc.py @@ -63,12 +63,22 @@ class TDDFTResult: energy_hartree: Ground-state SCF energy in Hartrees. homo_lumo_gap_ev: HOMO-LUMO gap in eV from the ground-state SCF, or ``None``. - converged: ``True`` only when BOTH the ground-state SCF converged - AND (if excited states were requested and the solve ran) every - requested TD root converged (AUDIT F08) — an SCF-only flag is - not overall success for a calculation whose deliverable is the - excited states. See ``td_converged``/``n_converged_states`` for - the per-root detail this folds together. + converged: ``True`` when the ground-state SCF converged, the TD + solve ran and produced states, and — whenever per-root status is + available — at least one requested root actually converged + (AUDIT F08; relaxed in code review from "every root" to "at + least one root", since it is routine for a Davidson solve's + higher/harder roots to miss the default iteration budget while + the lower, physically relevant ones are fine). ``False`` only + for a solve that raised, returned no states, or (when + ``td_converged`` is known) converged *none* of them — an + SCF-only flag is not overall success for a calculation whose + deliverable is the excited states. A PySCF build that doesn't + expose ``td.converged`` at all (``td_converged is None``) is + treated as "no per-root information available", not as + "unconverged" — it does not by itself flip this to ``False``. + See ``td_converged``/``n_converged_states`` for the per-root + detail this folds together. n_iterations: Number of ground-state SCF macro-iterations. method: DFT functional or HF method used. basis: Basis set. @@ -367,16 +377,30 @@ def _run_tddft_calc_body( except Exception: # noqa: BLE001 — cleanup (stream may be closed) pass - # AUDIT F08 — overall success requires every requested root to have - # actually converged, not just the ground-state SCF. A TD-DFT run whose - # entire purpose is the excited states is not "converged" if the - # Davidson solve raised before producing any roots, or if it returned - # roots that never converged. + # AUDIT F08 — overall success requires the TD solve to have actually + # produced converged roots, not just a ground-state SCF. A TD-DFT run + # whose entire purpose is the excited states is not "converged" if the + # Davidson solve raised before producing any roots, or if none of the + # roots it returned ever converged. + # + # Code review (2026-09): requiring *every* requested root to converge + # was too strict — a Davidson solve for nstates > a few routinely leaves + # the higher/harder roots short of the default iteration budget while + # the lower ones (usually what a UV-Vis analysis actually cares about) + # are fine, so a perfectly normal multi-state run was always flagged + # "treat with caution". Relaxed to "at least one requested root + # converged" — still catches the original bug this audit fixed (a + # solve where every root came back unconverged), and n_converged_states + # below still reports the exact per-root tally for anyone who wants it. + # + # Separately, when the installed PySCF doesn't expose ``td.converged`` + # at all, td_converged stays None — that is "no information", not + # "unconverged", and must not by itself force converged=False (it used + # to, forcing every TD-DFT/TDHF result on such a build to be flagged). converged = ( scf_converged and excitation_energies_ev != [] - and td_converged is not None - and all(td_converged) + and (td_converged is None or any(td_converged)) ) return TDDFTResult( diff --git a/tests/test_tddft_calc.py b/tests/test_tddft_calc.py index e7fb913..644c819 100644 --- a/tests/test_tddft_calc.py +++ b/tests/test_tddft_calc.py @@ -137,6 +137,85 @@ def test_converged_roots_report_full_convergence(self): assert result.n_converged_states == len(result.td_converged) assert result.converged is True + @pyscf_only + @pytest.mark.slow + def test_partial_root_convergence_is_not_treated_with_caution(self, monkeypatch): + """Code review (audit follow-up) — requiring *every* requested root + to converge was too strict: a Davidson solve routinely leaves + higher/harder roots short of the default iteration budget while the + lower, physically relevant ones are fine. A real (normal, fully + converged) solve is run, then per-root status is overwritten to a + realistic partial pattern — this isolates the post-processing + decision from Davidson's actual iteration behaviour, which is not + reliably reproducible across PySCF versions/platforms. + """ + import pyscf.scf.hf as pyscf_hf + import pyscf.tdscf.rhf # noqa: F401 — import side effect registers RHF.TDHF + + from quantui.tddft_calc import run_tddft_calc + + _original_tdhf_method = pyscf_hf.RHF.TDHF + + def _partially_converged_tdhf(self): + obj = _original_tdhf_method(self) + _original_kernel = obj.kernel + + def _patched_kernel(*args, **kwargs): + out = _original_kernel(*args, **kwargs) + # Lowest root converged, highest two did not — the common + # real-world pattern the audit's "every root" rule missed. + obj.converged = [True, False, False] + return out + + obj.kernel = _patched_kernel + return obj + + monkeypatch.setattr(pyscf_hf.RHF, "TDHF", _partially_converged_tdhf) + + result = run_tddft_calc(_water(), method="RHF", basis="6-31G", nstates=3) + + assert result.td_converged == [True, False, False] + assert result.n_converged_states == 1 + # At least one requested root converged, so the calculation as a + # whole is not "treat with caution" — n_converged_states still + # reports the exact per-root tally for anyone who wants it. + assert result.converged is True + + @pyscf_only + @pytest.mark.slow + def test_missing_td_converged_attribute_is_not_treated_as_unconverged( + self, monkeypatch + ): + """A PySCF build that doesn't expose ``td.converged`` at all must + not force every TD-DFT/TDHF result to "treat with caution" — that + is "no per-root information", not "unconverged".""" + import pyscf.scf.hf as pyscf_hf + import pyscf.tdscf.rhf # noqa: F401 — import side effect registers RHF.TDHF + + from quantui.tddft_calc import run_tddft_calc + + _original_tdhf_method = pyscf_hf.RHF.TDHF + + def _no_converged_attr_tdhf(self): + obj = _original_tdhf_method(self) + _original_kernel = obj.kernel + + def _patched_kernel(*args, **kwargs): + out = _original_kernel(*args, **kwargs) + obj.converged = None + return out + + obj.kernel = _patched_kernel + return obj + + monkeypatch.setattr(pyscf_hf.RHF, "TDHF", _no_converged_attr_tdhf) + + result = run_tddft_calc(_water(), method="RHF", basis="STO-3G", nstates=2) + + assert result.td_converged is None + assert result.n_converged_states is None + assert result.converged is True + if __name__ == "__main__": pytest.main([__file__, "-v", "--tb=short"]) From b41722a43b730d31ed868de74b7c122b876017ad Mon Sep 17 00:00:00 2001 From: QuantUI Bot Date: Mon, 7 Sep 2026 22:57:14 +0000 Subject: [PATCH 3/6] fix(history): stop blaming the SCF for a CC/TD/Hessian-only failure on saved-result cards (audit additional concerns) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit format_past_result still hardcoded a "SCF converged" row label, but data["converged"] for saved post-HF/frequency/TDDFT results already folds in CC-amplitude (F07), TD-root (F08), or Hessian (F15) convergence — exactly the mislabel those fixes corrected on the live cards (format_result/format_freq_result/format_tddft_result now say "Converged"). A saved CCSD result whose SCF converged but whose CC amplitudes did not rendered "SCF converged: No", wrongly implying the SCF itself failed; the same applied to frequency (Hessian-only) and TDDFT (TD-only) History cards. format_past_result now mirrors each live formatter's label choice: "Converged" for frequency and tddft (unconditional, since both always fold extra status in), "Converged" for a single-point card that carries cc_converged, and "SCF converged" otherwise. That label logic needs cc_converged/td_converged/n_converged_states to actually be present on saved result.json payloads — results_storage. save_result never persisted them (they existed only on the in-memory result object), so added them alongside the existing mp2/ccsd correlation fields. Also plugged the matching gap one hop upstream on the SLURM path: worker_payload.tddft_result_payload never serialized td_converged/n_converged_states into the staging JSON at all, and slurm_ingest._basic_result didn't forward them into the reconstructed result — both now round-trip through to History like cc_converged already did. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01E7tooXTTNTfemGJstUib8F --- quantui/app_formatters.py | 17 +++++- quantui/backends/slurm_ingest.py | 2 + quantui/backends/worker_payload.py | 11 ++++ quantui/results_storage.py | 19 ++++++ tests/test_app_formatters.py | 92 ++++++++++++++++++++++++++++++ tests/test_results_storage.py | 32 +++++++++++ 6 files changed, 172 insertions(+), 1 deletion(-) diff --git a/quantui/app_formatters.py b/quantui/app_formatters.py index de4602b..1d4de10 100644 --- a/quantui/app_formatters.py +++ b/quantui/app_formatters.py @@ -856,6 +856,21 @@ def format_past_result(data: dict[str, Any], result_dir: Optional[Path] = None) ) _conv = "Yes" if data.get("converged") else "No (treat results with caution)" _cc = _converged_color(bool(data.get("converged"))) + # AUDIT F07/F08/F15 (code review follow-up) — data["converged"] already + # folds in CC-amplitude (post-HF), TD-root (tddft), or Hessian + # (frequency) convergence on top of the reference SCF's own status, + # exactly like the live cards (format_result/format_freq_result/ + # format_tddft_result). A bare "SCF converged" label here would blame + # the SCF for a CC/TD/Hessian-only failure whose reference SCF was + # fine — mirror each live formatter's label choice. + if ct == "frequency": + _conv_label = "Converged" + elif ct == "tddft": + _conv_label = "Converged" + elif data.get("cc_converged") is not None: + _conv_label = "Converged" + else: + _conv_label = "SCF converged" _gap = ( f"{data['homo_lumo_gap_ev']:.4f} eV" if data.get("homo_lumo_gap_ev") is not None @@ -877,7 +892,7 @@ def format_past_result(data: dict[str, Any], result_dir: Optional[Path] = None) _gap, _theme.css.TEXT_HEADING, ), - ("SCF converged", _conv, _cc), + (_conv_label, _conv, _cc), ( "SCF iterations", ( diff --git a/quantui/backends/slurm_ingest.py b/quantui/backends/slurm_ingest.py index 5d59106..bfb30b5 100644 --- a/quantui/backends/slurm_ingest.py +++ b/quantui/backends/slurm_ingest.py @@ -79,6 +79,8 @@ def _basic_result(payload: dict[str, Any], record: JobRecord) -> SimpleNamespace ccsd_correlation_hartree=payload.get("ccsd_correlation_hartree"), ccsd_t_correction_hartree=payload.get("ccsd_t_correction_hartree"), cc_converged=payload.get("cc_converged"), + td_converged=payload.get("td_converged"), + n_converged_states=payload.get("n_converged_states"), dispersion_applied=payload.get("dispersion_applied"), solvent=payload.get("solvent"), gpu_used=bool(payload.get("gpu_used", False)), diff --git a/quantui/backends/worker_payload.py b/quantui/backends/worker_payload.py index 855c906..80e4b32 100644 --- a/quantui/backends/worker_payload.py +++ b/quantui/backends/worker_payload.py @@ -196,6 +196,17 @@ def tddft_result_payload(result) -> Dict[str, Any]: "scf_variant": getattr(result, "scf_variant", "") or None, # AUDIT F12 — was never serialized, though TDDFTResult carries it. "density_fit": bool(getattr(result, "density_fit", False)), + # AUDIT F08 (code review follow-up) — per-root convergence detail + # never left the worker process; a SLURM-submitted TDDFT run's + # History card could show only the folded "converged" bool, never + # the per-root tally format_tddft_result shows for an interactive + # run. + "td_converged": ( + [bool(c) for c in result.td_converged] + if getattr(result, "td_converged", None) is not None + else None + ), + "n_converged_states": getattr(result, "n_converged_states", None), "spectra": { "uv_vis": { "excitation_energies_ev": list(result.excitation_energies_ev), diff --git a/quantui/results_storage.py b/quantui/results_storage.py index d62a5f3..7864780 100644 --- a/quantui/results_storage.py +++ b/quantui/results_storage.py @@ -177,6 +177,16 @@ def _opt_str_list(x: object) -> Optional[list]: return None +def _opt_bool_list(x: object) -> Optional[list]: + """Coerce an optional iterable to a JSON-safe list of bools.""" + if x is None: + return None + try: + return [bool(v) for v in x] # type: ignore[union-attr, attr-defined] + except TypeError: + return None + + def _reorg_channels_payload(result) -> Optional[list]: """Serialise ReorgChannelResult objects, or None for other calc types. @@ -352,6 +362,15 @@ def save_result( "ccsd_t_correction_hartree": _opt_float( getattr(result, "ccsd_t_correction_hartree", None) ), + # AUDIT F07/F08 (code review follow-up) — cc_converged/td_converged + # were only ever attributes on the in-memory result object, never + # persisted here, so a reloaded History card had no way to tell "no + # post-HF/TD-DFT correlation ran" (None) apart from "it ran and + # converged/didn't" (True/False) even though data["converged"] + # already folds one of these in. See format_past_result. + "cc_converged": getattr(result, "cc_converged", None), + "td_converged": _opt_bool_list(getattr(result, "td_converged", None)), + "n_converged_states": _opt_int(getattr(result, "n_converged_states", None)), # Persisted so the saved-result card matches the live card # (formatter-parity fix). Additive — absent on older results, where the # history card falls back exactly as before (CPU / no dipole / no diff --git a/tests/test_app_formatters.py b/tests/test_app_formatters.py index 76b53e4..a6e8ca2 100644 --- a/tests/test_app_formatters.py +++ b/tests/test_app_formatters.py @@ -436,6 +436,98 @@ def test_format_past_result_hf_dft_has_no_breakdown(): assert "HF reference" not in html +# --------------------------------------------------------------------------- +# Code review (audit follow-up) — format_past_result's convergence-row +# label must not blame the SCF for a CC-only/TD-only/Hessian-only failure, +# mirroring format_result/format_freq_result/format_tddft_result (AUDIT +# F07/F08/F15). +# --------------------------------------------------------------------------- + + +def test_format_past_result_hf_dft_labels_scf_converged(): + """A plain HF/DFT single point has nothing else folded into + ``converged`` — "SCF converged" is still accurate.""" + data = { + "calc_type": "single_point", + "converged": True, + "homo_lumo_gap_ev": 10.0, + "energy_hartree": -75.0, + "energy_ev": -2040.0, + "n_iterations": 10, + "timestamp": "2026-05-02_12-00-00-000001", + "formula": "H2O", + "method": "RHF", + "basis": "STO-3G", + } + html = format_past_result(data) + assert "SCF converged" in html + + +def test_format_past_result_ccsd_failure_does_not_blame_scf(): + """A saved CCSD result whose reference SCF converged but whose CC + amplitudes did not must not render "SCF converged: No" — that + misleadingly implies the SCF itself failed.""" + data = { + "calc_type": "single_point", + "converged": False, + "homo_lumo_gap_ev": 27.0, + "energy_hartree": -75.0139, + "energy_ev": -2041.23, + "n_iterations": 5, + "timestamp": "2026-06-10_15-48-02-285574", + "formula": "H2O", + "method": "CCSD", + "basis": "STO-3G", + "ccsd_correlation_hartree": -0.0497, + "cc_converged": False, + } + html = format_past_result(data) + assert "SCF converged" not in html + assert "Converged" in html + assert "No (treat results with caution)" in html + + +def test_format_past_result_frequency_labels_converged_not_scf(): + """AUDIT F15's folded Hessian status must not render "SCF converged" + even when a CC/TD sub-flag isn't present.""" + data = { + "calc_type": "frequency", + "converged": False, + "homo_lumo_gap_ev": 27.0, + "energy_hartree": -74.9, + "energy_ev": -2039.0, + "n_iterations": 12, + "timestamp": "2026-06-10_15-48-02-285574", + "formula": "H2O", + "method": "RHF", + "basis": "STO-3G", + "spectra": {"ir": {"frequencies_cm1": [], "ir_intensities": []}}, + } + html = format_past_result(data) + assert "SCF converged" not in html + assert "Converged" in html + + +def test_format_past_result_tddft_labels_converged_not_scf(): + """AUDIT F08's folded per-root TD status must not render "SCF + converged".""" + data = { + "calc_type": "tddft", + "converged": False, + "homo_lumo_gap_ev": 12.0, + "energy_hartree": -75.0, + "energy_ev": -2040.0, + "n_iterations": 8, + "timestamp": "2026-06-10_15-48-02-285574", + "formula": "H2O", + "method": "RHF", + "basis": "STO-3G", + } + html = format_past_result(data) + assert "SCF converged" not in html + assert "Converged" in html + + # --------------------------------------------------------------------------- # M-UX2 UXP2.10 — results panel labels RKS vs UKS # --------------------------------------------------------------------------- diff --git a/tests/test_results_storage.py b/tests/test_results_storage.py index 4551a8e..8e80084 100644 --- a/tests/test_results_storage.py +++ b/tests/test_results_storage.py @@ -172,6 +172,38 @@ def test_each_call_creates_unique_directory(self, tmp_path): d2 = save_result(_make_result(), results_dir=tmp_path) assert d1 != d2 + def test_cc_converged_persisted(self, tmp_path): + """AUDIT F07 (code review follow-up) — cc_converged was carried on + SessionResult but never round-tripped to result.json, so a reloaded + History card had no way to tell "no post-HF ran" (None) apart from + "ran and didn't converge" (False), even though data["converged"] + already folds it in (see format_past_result).""" + saved = save_result(_make_result(cc_converged=False), results_dir=tmp_path) + data = json.loads((saved / "result.json").read_text()) + assert data["cc_converged"] is False + + def test_cc_converged_null_when_absent(self, tmp_path): + saved = save_result(_make_result(), results_dir=tmp_path) + data = json.loads((saved / "result.json").read_text()) + assert data["cc_converged"] is None + + def test_td_converged_and_n_converged_states_persisted(self, tmp_path): + """AUDIT F08 (code review follow-up) — same gap as cc_converged, + for TD-DFT's per-root convergence detail.""" + saved = save_result( + _make_result(td_converged=[True, False, False], n_converged_states=1), + results_dir=tmp_path, + ) + data = json.loads((saved / "result.json").read_text()) + assert data["td_converged"] == [True, False, False] + assert data["n_converged_states"] == 1 + + def test_td_converged_null_when_absent(self, tmp_path): + saved = save_result(_make_result(), results_dir=tmp_path) + data = json.loads((saved / "result.json").read_text()) + assert data["td_converged"] is None + assert data["n_converged_states"] is None + class TestSaveResultJsonSafeCoercion: """L audit fix: save_result must coerce every numeric/boolean field to a From ccddaf105bdf52430ecf0e2dd63bfedd7c54e535 Mon Sep 17 00:00:00 2001 From: QuantUI Bot Date: Mon, 7 Sep 2026 22:57:20 +0000 Subject: [PATCH 4/6] perf(cube): cache per-element ECP core-electron lookups in infer_charge_and_spin (audit additional concerns) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit infer_charge_and_spin imported pyscf.gto and called load_ecp inside the per-atom loop, so an N-atom molecule with a basis argument repeated both the import and the ECP-table lookup once per atom instead of once per unique element — needless repeated work on the interactive cube-export "Generate" path for a large molecule. The pyscf.gto import now runs at most once (only when a basis is given), and each unique element's core-electron count is computed once and cached, with every atom of that element served from the cache. Added a test that counts load_ecp calls per element symbol. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01E7tooXTTNTfemGJstUib8F --- quantui/orbital_visualization.py | 35 ++++++++++++++++++++--------- tests/test_orbital_visualization.py | 27 ++++++++++++++++++++++ 2 files changed, 51 insertions(+), 11 deletions(-) diff --git a/quantui/orbital_visualization.py b/quantui/orbital_visualization.py index 0fa391d..5e5af4e 100644 --- a/quantui/orbital_visualization.py +++ b/quantui/orbital_visualization.py @@ -592,20 +592,33 @@ def infer_charge_and_spin( spin = int(np.sum(np.isclose(occ, 1.0))) n_electrons = float(occ.sum()) + # Code review — the ``pyscf.gto`` import and the ``load_ecp`` table + # lookup used to run once per atom instead of once per unique element, + # even though every atom of the same element gets the same answer. For + # a large cube-export molecule (the interactive "Generate" path this + # feeds) that's needless repeated work: hoist the import out of the + # loop and cache the per-element core-electron count. + _ecp_core_electrons: dict = {} + if basis: + from pyscf import gto as _gto + + def _core_electrons_for(sym: str) -> int: + if not basis: + return 0 + if sym in _ecp_core_electrons: + return _ecp_core_electrons[sym] + try: + _ecp_data = _gto.basis.load_ecp(basis, sym) + core_electrons = int(_ecp_data[0]) if _ecp_data else 0 + except Exception: + core_electrons = 0 + _ecp_core_electrons[sym] = core_electrons + return core_electrons + nuclear_charge = 0 for sym, _pos in mol_atom: z = ATOMIC_NUMBERS.get(sym, 0) - core_electrons = 0 - if basis: - try: - from pyscf import gto as _gto - - _ecp_data = _gto.basis.load_ecp(basis, sym) - if _ecp_data: - core_electrons = int(_ecp_data[0]) - except Exception: - core_electrons = 0 - nuclear_charge += z - core_electrons + nuclear_charge += z - _core_electrons_for(sym) charge = int(round(nuclear_charge - n_electrons)) return charge, spin diff --git a/tests/test_orbital_visualization.py b/tests/test_orbital_visualization.py index 6402b90..6edcb7d 100644 --- a/tests/test_orbital_visualization.py +++ b/tests/test_orbital_visualization.py @@ -162,6 +162,33 @@ def test_all_electron_basis_unaffected_by_ecp_lookup(self): charge, spin = infer_charge_and_spin(mol_atom, occ, basis="STO-3G") assert (charge, spin) == (0, 0) + def test_ecp_lookup_is_cached_per_unique_element(self, monkeypatch): + """Code review — the pyscf.gto import and load_ecp table lookup + used to run once per atom instead of once per unique element, even + though every atom of the same element gets the same answer.""" + pytest.importorskip("pyscf") + from pyscf import gto + + calls: list = [] + _original_load_ecp = gto.basis.load_ecp + + def _counting_load_ecp(basis, sym): + calls.append(sym) + return _original_load_ecp(basis, sym) + + monkeypatch.setattr(gto.basis, "load_ecp", _counting_load_ecp) + + occ = [2.0, 2.0, 2.0] + mol_atom = [ + ("Na", [0, 0, 0]), + ("Na", [0, 0, 3.0]), + ("H", [0, 0, 6.0]), + ] + infer_charge_and_spin(mol_atom, occ, basis="LANL2DZ") + + assert calls.count("Na") == 1 + assert calls.count("H") == 1 + def test_none_inputs_return_zero_zero(self): assert infer_charge_and_spin(None, [2.0]) == (0, 0) assert infer_charge_and_spin([("H", [0, 0, 0])], None) == (0, 0) From a5b726331c6774cd3a5357ef895b3da2d1406c81 Mon Sep 17 00:00:00 2001 From: QuantUI Bot Date: Wed, 9 Sep 2026 00:02:59 +0000 Subject: [PATCH 5/6] fix(ci): satisfy mypy's no-any-return on the ECP core-electron cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit quantui/orbital_visualization.py:609 — `_ecp_core_electrons: dict = {}` left the dict untyped, so mypy inferred `dict[Any, Any]` and flagged `_ecp_core_electrons[sym]` as returning `Any` from a function declared `-> int` (no-any-return), failing CI's Lint & type check job (mypy~=1.10.0, python_version=3.9 per pyproject.toml). Typed the cache as `dict[str, int]`. Verified against the pinned mypy==1.10.1 with the repo's exact warn_return_any/python_version=3.9 config on an isolated repro of the function — no issues. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01E7tooXTTNTfemGJstUib8F --- quantui/orbital_visualization.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/quantui/orbital_visualization.py b/quantui/orbital_visualization.py index 5e5af4e..cc16829 100644 --- a/quantui/orbital_visualization.py +++ b/quantui/orbital_visualization.py @@ -598,7 +598,7 @@ def infer_charge_and_spin( # a large cube-export molecule (the interactive "Generate" path this # feeds) that's needless repeated work: hoist the import out of the # loop and cache the per-element core-electron count. - _ecp_core_electrons: dict = {} + _ecp_core_electrons: dict[str, int] = {} if basis: from pyscf import gto as _gto From f1bed2ae21dfc9ec530bdd12d785b86d39dbc9b7 Mon Sep 17 00:00:00 2001 From: QuantUI Bot Date: Wed, 9 Sep 2026 00:12:02 +0000 Subject: [PATCH 6/6] fix(cube): restore exception safety around the hoisted pyscf.gto import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The mypy fix's caching refactor (a5b7263) hoisted `from pyscf import gto as _gto` out of the per-atom loop, but moved it out of the try/except that used to wrap it — the original code caught ANY failure importing or using pyscf.gto and fell back to the all-electron count, whereas the hoisted import let such a failure raise straight out of infer_charge_and_spin. This was not just theoretical: CI's Windows job hit it twice in a row on this branch (test_render_orbital_isosurface_uses_snapshotted_method_not_ live_dropdown, which calls infer_charge_and_spin(mol_atom, mo_occ, basis="sto-3g") on the way to a mocked generate_cube_from_arrays) — identical KeyError: 'method' both times, because the render call never reached its mocked generate_cube_from_arrays call. The base branch's own CI run (PR #120) passed the same test cleanly, before this refactor existed, which rules it out as a pre-existing flake; this diff is the one thing that changed. Wrapped the import in try/except again (falling back to `_gto = None`, which `_core_electrons_for` already treats as "no ECP data available" → 0 core electrons), so a transient/failed pyscf.gto import can no longer propagate out of this function — restoring the original safety net while keeping the import-once/cache-per-element behavior from the code-review fix. Also avoids mypy's `no-redef` by importing under a throwaway name and assigning it to `_gto` rather than reusing `_gto` as the import alias in both branches. Verified against the pinned mypy==1.10.1 with the repo's exact warn_return_any/python_version=3.9 config on an isolated repro — no issues. ruff + black clean. tests/test_orbital_visualization.py and the orbital/isosurface subset of tests/test_app.py green locally (Linux; the failure was Windows-specific). Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01E7tooXTTNTfemGJstUib8F --- quantui/orbital_visualization.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/quantui/orbital_visualization.py b/quantui/orbital_visualization.py index cc16829..90526a4 100644 --- a/quantui/orbital_visualization.py +++ b/quantui/orbital_visualization.py @@ -599,11 +599,23 @@ def infer_charge_and_spin( # feeds) that's needless repeated work: hoist the import out of the # loop and cache the per-element core-electron count. _ecp_core_electrons: dict[str, int] = {} + _gto: Any = None if basis: - from pyscf import gto as _gto + try: + import pyscf.gto as _gto_mod + + _gto = _gto_mod + except Exception: + # Match the pre-refactor behavior: any failure to even import + # pyscf.gto (not just a load_ecp lookup failure) falls back to + # the all-electron count rather than raising out of this + # function — infer_charge_and_spin has no PySCF hard dependency + # otherwise, and a caller (e.g. cube-export's Generate path) + # must not crash just because ECP data couldn't be resolved. + _gto = None def _core_electrons_for(sym: str) -> int: - if not basis: + if not basis or _gto is None: return 0 if sym in _ecp_core_electrons: return _ecp_core_electrons[sym]