Fix 22 audit findings across calculation, UI, and packaging - #120
Merged
Conversation
…d (AUDIT F01) PySCF's thermo() returns S_tot in Eh/K, not J/(mol·K). freq_calc.py stored that raw Eh/K number directly as ThermoData.S_jmol, then divided by _HARTREE_TO_JMOL a second time when computing G = H - T*S — deflating the entropy term by ~2.6e6x and leaving G numerically indistinguishable from H (a +56.21 kJ/mol error for RHF/STO-3G water, per the GPT-Astra audit's F01). Now the Eh/K value is used directly for G = H - T*S_Eh_per_K (dimensionally consistent with H_hartree, no conversion factor needed), and converted to J/(mol·K) only for the stored/displayed S_jmol. Added a regression test with an independent PySCF reference (S=188.538424 J/(mol·K), G=-74.954908540 Eh for RHF/STO-3G water) rather than only the existing sign checks (S > 0, G < H), which the old bug also passed. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…ion (AUDIT F02) CPU Raman activities were ~45.54x too large. The finite-difference polarizability derivative is d(alpha[a0^3])/d(x[Bohr]); the old code divided by a single BOHR_TO_ANGSTROM, which only rescales the denominator (Bohr -> Angstrom) and leaves the numerator in a0^3 instead of Angstrom^3. The missing BOHR_TO_ANGSTROM**3 factor in the derivative becomes BOHR_TO_ANGSTROM**6 once squared into the Placzek invariants (S = 45*abar^2 + 7*gamma^2), matching the audit's measured ~45.54x inflation. Now the derivative is multiplied by BOHR_TO_ANGSTROM**2, combining both the numerator and denominator conversions correctly. Added an independent regression test for H2/RHF/STO-3G: rather than only checking existing sign/relative-magnitude assertions (which the old bug also passed), it central-differences the real SCF polarizability directly along the mode's own normal-mode vector (bypassing raman_calc.py's atom-by-atom Jacobian entirely) and checks the result matches — reproducing the audit's independent finite-difference method and its ~22.76 A^4/amu reference value. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…AUDIT F03) wB97X-D was aliased to bare "wb97x" with an external Grimme D3 wrapper, as a workaround for PySCF rejecting "wb97x-d" (blacklisted as an ambiguous short alias in pyscf.scf.dispersion.parse_dft). But this silently computes a different functional: bare wb97x has range-separation omega=0.3 and different short-range exact exchange than the real wB97X-D (omega=0.2), confirmed via pyscf.dft.libxc.rsh_coeff. Results, exports, and labels kept the "wB97X-D" name regardless. The actual Chai & Head-Gordon (2008) wB97X-D functional is available under its full LibXC name, hyb_gga_xc_wb97x_d, which PySCF's short-alias blacklist does not intercept — and it already has its own built-in empirical dispersion, so no external D3 wrapper is needed (removed from _NEEDS_D3; applying Grimme D3 on top would double-count dispersion). Verified SCF, analytic gradient, and analytic Hessian all work with this xc string. Fixed in both quantui/session_calc.py (used by every DFT entry point) and the duplicated table in config.py's exported-script template. Updated tests/test_xc_resolution.py's expectations to match (it previously asserted the buggy bare-wb97x substitution as correct behavior), and added a numeric regression confirming the resolved functional's omega differs from bare wb97x per an independent PySCF/LibXC lookup. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…(AUDIT F04) maybe_apply_d3() silently returned the original (uncorrected) mf on a missing pyscf.dftd3, with no structured field anywhere recording that the result is missing its dispersion correction, and results kept labels like "PBE-D3" regardless. The optimizer's call site didn't even pass progress_stream, so a missing dftd3 gave no warning at all on that path — unlike every other DFT entry point. maybe_apply_d3() now returns (mf, dispersion_applied): True when D3 was applied, False when the method needs it but pyscf.dftd3 is unavailable, None when the method doesn't use D3. It always logs a warning (visible regardless of progress_stream), in addition to writing to progress_stream when one is given. All 5 call sites (session_calc, freq_calc, tddft_calc, nmr_calc, optimizer) updated for the new return signature; the optimizer call now also passes its progress_stream. SessionResult and OptimizationResult gain a dispersion_applied field so a PBE-D3 result that silently lost its correction is distinguishable from one that got it; SessionResult.summary() surfaces an explicit warning line when dispersion_applied is False rather than reading like an ordinary corrected result. (wB97X-D no longer touches this path at all after the F03 fix — its dispersion is built into the functional, so _NEEDS_D3 now contains only PBE-D3, considerably narrowing this bug's remaining blast radius.) Updated tests/test_xc_resolution.py's TestMaybeApplyD3 for the new tuple return, and added a regression confirming the warning is logged even with no progress_stream (the specific gap on the optimizer path). Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
… (AUDIT F05)
Both freq_ir_workers.run_displaced_scf and freq_raman_workers.run_displaced_
polarizability rebuild their Mole from only atom string / basis / charge /
spin, dropping mol.ecp that the reference calculation set. For a
heavy-element system (e.g. NaH/LANL2DZ), the workers ran a 12-electron
all-electron calculation instead of the correct 2-explicit-electron ECP
one — a different Hamiltonian, not numerical noise. The serial IR/Raman
loops in freq_calc.py/raman_calc.py were unaffected (they reuse the same
already-ECP-bearing mol object).
Both init_worker()/init_raman_worker() now take an optional ecp mapping
(the reference mol's mol.ecp, a plain {element: basis} dict of strings —
trivially picklable across the ProcessPoolExecutor boundary), stored in
worker state and applied via mol.ecp before mol.build(). The two
ProcessPoolExecutor call sites (freq_calc.py, raman_calc.py) now pass
mol.ecp through initargs.
Added regression tests exercising the real worker functions directly
(mirroring the audit's own NaH/LANL2DZ reproduction): with the correct ecp
mapping the worker's SCF matches the true 2-electron system; with ecp
omitted (the old behavior) it silently runs the wrong 12-electron one,
giving a materially different dipole/polarizability on the same geometry.
Contributions:
- Claude (Sonnet 5): code edits, review, and conceptual discussion
- Jonathan Schultz: overall vision, planning, review, and orchestration
Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…T F06)
The standalone script template set mol.basis but never mol.ecp. Executing
the exported NaH/LANL2DZ RHF script reported 12 electrons and
E = -20.13 Ha (all-electron) instead of the correct 2-explicit-electron
ECP result — a completely different Hamiltonian, not a meaningful
correlation or binding-energy difference. This made an exported script
scientifically inconsistent with the in-app calculation it's supposed to
reproduce.
generate_calculation_script() now computes the same ecp_for_basis(basis,
molecule.atoms) mapping the in-app code paths use and embeds it as a
literal dict via mol.ecp = {ecp!r} — {} for an all-electron basis,
{'Na': 'LANL2DZ'} etc. for one that carries an ECP.
Added a regression that executes the generated NaH/LANL2DZ script as a
real subprocess (no QuantUI import, matching how a student would run it)
and confirms it reports 2 electrons, not 12; plus a static check that the
ECP mapping is embedded (present and correct for a heavy-element basis,
explicitly {} for an all-electron one) rather than simply absent.
Contributions:
- Claude (Sonnet 5): code edits, review, and conceptual discussion
- Jonathan Schultz: overall vision, planning, review, and orchestration
Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…t (AUDIT F07) result.converged came solely from the HF reference (mf.converged); _ccsd_obj.converged was never checked, so a real one-iteration-limited CCSD solve (converged=False, E_corr=-0.0449... for water/STO-3G) was reported as converged=True. CCSD(T) also proceeded to the perturbative triples correction regardless of whether the CCSD amplitudes themselves converged, and post-HF work (MP2/CCSD/CCSD(T)) launched even when the reference SCF itself failed every rescue attempt. Now: - MP2/CCSD/CCSD(T) are skipped entirely (with a status message) when the reference SCF didn't converge, rather than running correlation on top of a wrong Hamiltonian. - CCSD's own amplitude convergence is tracked in a new SessionResult field, cc_converged, separate from the HF reference's. - CCSD(T) triples are skipped when CCSD itself didn't converge. - The overall converged field folds in cc_converged for CCSD/CCSD(T) methods, so a result can no longer read "converged" when either stage failed. summary() surfaces an explicit warning when CC didn't converge. Added regression tests: a real one-iteration-limited CCSD solver (genuine PySCF kernel, only max_cycle forced) confirming converged=False/cc_converged=False now propagates instead of being swallowed, and a check that CCSD is skipped (no correlation computed) when the reference SCF is unconverged. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…lation (AUDIT F08) td.kernel() copied excitation energies/oscillator strengths without ever checking td.converged (a per-root array from the Davidson solve); the single success flag on TDDFTResult came solely from the ground-state SCF. A real one-iteration-limited TDHF/6-31G water solve had converged=[False, False, False] yet was reported as a converged result with three excitations (9.804744, 11.993916, 12.420652 eV). TDDFTResult gains td_converged (per-root flags) and n_converged_states, distinct from the ground-state SCF's own status. converged is now True only when the SCF converged AND the TD solve ran AND every requested root converged — a calculation whose entire deliverable is the excited states is not "converged" just because the reference SCF was fine. Also fixed the same "SCF converged" mislabeling this change introduces: SessionResult.converged already folds in CCSD's own convergence (AUDIT F07), so format_result()'s card now says "Converged" rather than "SCF converged" whenever a CC method ran; format_tddft_result() likewise says "Converged" (reflecting the combined status) and shows the converged-root count alongside the states-computed count when they differ. Added a controlled reproduction test (real PySCF SCF + TDHF kernels, only max_cycle forced to 1 via RHF.TDHF's registered class_as_method) matching the audit's exact numbers, plus a happy-path sanity check that a normal solve reports full per-root convergence. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
_QuantUIPySCFCalc.calculate() ran run_scf_with_rescue() but never checked mf.converged before computing/returning the analytic gradient — BFGS could satisfy its force criterion using an invalid electronic solution. A real H2 optimization near its minimum, with each SCF limited to one cycle and rescue disabled, reported converged=True after three steps despite all four SCF evaluations being unconverged. Since BFGS's Hessian update chains each step's forces into the next, one bad point corrupts the rest of the trajectory too, not just that step. _QuantUIPySCFCalc now raises when mf.converged is False after run_scf_with_rescue has already exhausted every rescue stage, rather than handing ASE a physically meaningless gradient. This is caught by optimize_geometry()'s existing outer exception handler and surfaces as a clear "Geometry optimization failed" error; the batch worker's pre-opt path already falls back to the input geometry on any such failure. pes_scan.py reuses the same calculator for both its constrained-relaxation scan points and its diatomic-bond points (which hard-code ok=True since there's no relaxable DOF to check) — this fix closes the diatomic-scan gap too: an SCF failure there now raises, is caught by the scan loop's existing per-point exception handler, and correctly marks that point NaN/converged_all=False instead of the electronic failure being invisible behind the trivial "no geometry to relax" ok=True. Added controlled reproductions for both paths (real PySCF SCF/gradient kernels, only max_cycle forced to 1 via a monkeypatched RHF factory) — one confirming optimize_geometry raises, one confirming a diatomic PES scan point is marked failed rather than silently accepted. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…AUDIT F10) CalcIdentity.resume_key included molecule/method/basis/geometry/calc_type but not WHICH internal coordinate a PES scan targets — calc_type was just "pes_scan" for every scan configuration, so a bond scan and an angle scan of the same starting molecule/method/basis collided on the same resume_key and could resume into each other's checkpoint directory. Separately, per-point reuse (_reuse_scan_point) validated only the scalar scan target value against the cached record, not which bond/ angle/dihedral (or which atoms) that value belonged to. A banked O-H scan point at 1.0 A was reused for a fresh H-H scan targeting 1.0 A because both compared equal on that scalar value alone, returning the O-H point's energy under an H-H distance that was never actually 1.0 A. Fixes: - CalcIdentity gains an `extra` tuple of calc-type-specific discriminators, folded into resume_key (not warm_start_key — an SCF density is still a good initial guess across different scan configurations of the same system). Both the interactive app (app_runflow.checkpoint_identity) and the batch worker (_begin_worker_checkpoint) now pass (scan_type, *atom_indices) for PES scans. - Each banked point now records its own scan_type/atom_indices; _reuse_scan_point rejects a record whose scan_type or atom_indices don't match the current scan. - Defense in depth: _reuse_scan_point also measures the ACTUAL coordinate from the restored geometry (via ASE get_distance/ get_angle/get_dihedral) and rejects the point if that measured value disagrees with the target — catching a mismatched record even if its own scan_type/atom_indices fields were themselves stale or absent (an older checkpoint schema). Added regression tests: a real end-to-end run_pes_scan() reproducing the audit's scenario (a bogus cached point recorded under a different scan_type, at the exact value the real scan targets, must be recomputed rather than returned), plus unit tests for the scan_type/atom_indices/ actual-coordinate mismatches at the _reuse_scan_point level, using an ASE-Atoms-alike stub whose get_distance/get_angle/get_dihedral are real geometry (not hardcoded), so a genuinely wrong coordinate is caught. Two existing tests (test_backends_worker.py, which pre-built a CalcIdentity without the new extra field) updated to match the new resume_key formula. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…AUDIT F11) The "Implicit solvent (PCM)" checkbox stayed enabled for every calc type, but run_freq_calc, run_tddft_calc, run_nmr_calc, and run_pes_scan don't accept a solvent argument at all — a live call-through probe confirmed Frequency's backend received only molecule/method/basis/progress_stream regardless of the checkbox. Checking it for Frequency, UV-Vis (TD-DFT), NMR Shielding, or PES Scan was a complete no-op: the result was gas-phase with no indication anything was skipped. Interactive app (app_runflow.on_calc_type_changed): the solvent checkbox is now disabled and unchecked for those four calc types, with its description explaining why. Single Point (full PCM support), Geometry Opt, and Reorganization Energy (both a real gas-phase-optimization + solvated-final-single-point approximation, not silently ignored) keep it enabled; the latter two now say so explicitly in the checkbox label rather than implying a fully solvated optimization. Batch worker: request.solvent was already threaded through only to _run_single_point and _run_reorganization_energy (correctly scoped — _run_geometry_opt/_run_frequency/_run_tddft/_run_nmr/_run_pes_scan never received it), but nothing rejected a solvent set for an unsupported calc_type — it was simply dropped. run_worker_request now fails such a request with an actionable UNSUPPORTED_CAPABILITY error instead. Added tests: UI gating (checkbox disables/unchecks on switching to an unsupported calc type, re-enables without silently re-checking on switching back) and the new batch-side validation (rejects solvent on each unsupported calc_type, accepts it on single_point). Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…(AUDIT F12) Two layers of loss between a batch worker's result and its saved History entry: 1. Serialization gap (worker_payload.py): session_result_payload() never included mp2/ccsd correlation energies, cc_converged, dispersion_applied, solvent, gpu_used/gpu_name, or density_fit at all, even though SessionResult computes all of them. freq/tddft/nmr_result_payload() likewise never serialized density_fit despite their result types carrying it. 2. Ingest gap (slurm_ingest.py): _basic_result() reconstructed only 7 generic fields (energy, gap, converged, n_iterations, method, basis, formula) from the staging JSON, even for the fields session_result_payload() DID already serialize (Mulliken charges, dipole, atom symbols, SCF variant/rescue provenance). save_result() reads everything via getattr(result, ..., default), so the missing attributes on the reconstructed SimpleNamespace silently became null in result.json regardless of what the JSON actually held — a real water round trip lost the dipole (1.725515 D), Mulliken charges, atom symbols, and RHF provenance. Both layers fixed: session_result_payload() and freq/tddft/nmr_result_ payload() now serialize the complete field set their result types carry; _basic_result() now reconstructs every field save_result() reads, defensively via .get() so a field absent for a given calc type still round-trips as the same None/default save_result already treats as "not applicable." Added a full worker -> staging JSON -> ingest -> result.json regression test (mirroring the audit's own water reproduction) confirming Mulliken charges, dipole, atom symbols, SCF variant, post-HF correlation, and solvent/GPU/DF metadata all survive; plus direct unit tests for each newly-serialized field on session/freq/tddft/nmr_result_payload(). Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…branch (AUDIT F13) The exported-script template handled only RHF and UHF explicitly; every other method (including MP2, CCSD, CCSD(T)) fell into the DFT branch, which set mf.xc = 'MP2'/'CCSD'/'CCSD(T)' directly and failed with "LibXCFunctional: name '...' not found" — RHF succeeded in the same harness, but the three post-HF methods all errored on execution. Added an explicit branch for MP2/CCSD/CCSD(T): the reference is scf.RHF(mol) regardless of spin (a factory that dispatches to true RHF for closed-shell and ROHF for open-shell), matching quantui/session_calc.py's post-HF reference dispatch exactly. Post-HF correlation mirrors session_calc.py's own AUDIT F07 fix too: MP2/CCSD only run on a converged reference, and CCSD(T) triples only run if CCSD's own amplitudes converged — the exported script's overall success/exit-code now reflects both, and the saved results.npz gains the correlation breakdown (mp2_correlation_hartree / ccsd_correlation_hartree / ccsd_t_correction_hartree / cc_converged) instead of only the summed total energy. Added a regression executing the real generated water/STO-3G script for each of the three methods (real subprocess, no QuantUI import) and checking the saved .npz's correlation fields are negative and cc_converged is true — not just exit code 0, which a script that silently skipped correlation would also pass. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…DIT F14) infer_charge_and_spin() assumed a 1-D MO occupation array meant closed-shell (spin=0), but ROHF is also 1-D (values 2/1/0, with singly-occupied orbitals). A real OH doublet's occupations [2,2,2,2,1,0] inferred (charge, spin)=(0,0) — impossible for 9 electrons — and cube generation then failed. It also computed nuclear charge as a bare atomic-number sum, which overcounts an ECP system by however many core electrons the ECP replaced: NaH/LANL2DZ (2 explicit electrons) inferred +10 instead of neutral. Fixes: - spin (for a 1-D occupation array) is now the count of singly-occupied orbitals (occ == 1) — 0 for genuine closed-shell RHF/RKS/MP2/CCSD/ CCSD(T) references (no singly-occupied orbitals), and correct for ROHF, by the standard convention that every ROHF singly-occupied orbital is alpha (so that count IS 2S = n_alpha - n_beta directly). The 2-D (UHF/UKS) path is unchanged. - infer_charge_and_spin() takes an optional basis argument; when given, it looks up each element's ECP core-electron count (pyscf.gto.basis. load_ecp(basis, symbol)[0], the same lookup inorganic_guards. ecp_for_basis uses) and subtracts it from that element's atomic number before summing, so the effective nuclear charge matches what the ECP calculation actually used. Both call sites (app_visualization.py's live cube generation, orbital_visualization.py's exported-NPZ cube path) now pass the basis they already have in scope. Rebuilt cube/Molden molecules still don't restore full ECP metadata (AO sampling can reuse the same basis coefficients regardless), so this fixes charge/core provenance specifically — not a claim that every ECP orbital amplitude was previously wrong. Added regression tests reproducing both of the audit's exact scenarios (OH doublet ROHF occupations -> (0, 1); NaH/LANL2DZ -> (0, 0) with the basis given, (10, 0) without it as the documented pre-fix fallback when there's no basis to resolve the ECP from), plus closed-shell/UHF/ all-electron-basis sanity checks. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…IT F15)
A real OH/RHF/STO-3G request dispatches to ROHF (open-shell), whose
analytic Hessian is unavailable on this PySCF path. The whole Hessian/
harmonic-analysis/IR/Raman/thermo block is one big try/except; when
mf.Hessian() raised, the caught exception left FreqResult.converged
reading whatever the reference SCF alone reported — a real reproduction
gave converged=True with frequencies_cm1=[]. A frequency calculation with
no computed Hessian is not a successful frequency analysis, whatever the
SCF did.
FreqResult.converged is now True only when BOTH the SCF converged AND the
Hessian/harmonic-analysis step actually completed (tracked via a
_hessian_completed flag set right after harmonic_analysis() succeeds, so
the later best-effort IR/Raman/thermo enrichment doesn't gate it).
TD-DFT's half of this same pattern (an exception during the TD solve
leaving excitation lists empty but still reading converged) is already
covered by the AUDIT F08 fix: converged there already requires the TD
solve to have actually produced roots.
Also fixed the resulting "SCF converged" mislabeling this creates: since
.converged now reflects more than just the reference SCF for FreqResult/
TDDFTResult/CCSD-bearing SessionResult, app_formatters.py's freq result
card and log_utils.py's plain-text log footer both say "Converged"/
"Result {converged|did NOT converge}" instead of claiming "SCF" — a
Hessian failure with a genuinely converged SCF would otherwise misleadingly
blame the reference SCF.
Added a regression reproducing the audit's exact scenario: a real OH
doublet RHF/STO-3G run (dispatches to ROHF) now reports scf_variant=ROHF,
frequencies_cm1=[], and converged=False.
Contributions:
- Claude (Sonnet 5): code edits, review, and conceptual discussion
- Jonathan Schultz: overall vision, planning, review, and orchestration
Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…(AUDIT F16) app.py opens _ckpt and resolves _resume once per run (used by Geometry Opt, PES Scan, and Reorganization Energy), but the interactive Frequency call to run_freq_calc() never received either — despite freq_calc.py supporting displacement-level checkpointing (M-CHECKPOINT CHK.4) and the batch frequency route already using it. An interrupted interactive Frequency run therefore could never bank or resume its expensive IR/Raman displacement SCFs, unlike every other checkpoint-aware calc type. Passes checkpoint=_ckpt, resume=_resume through, matching the existing pattern at the other three call sites. The existing wiring test only grepped app.py for occurrences of "checkpoint=_ckpt" across the whole file — a count that stayed >= 3 with or without Frequency's own call site having it, so it never caught this gap. Added a test that drives the real _do_run() dispatch for the Frequency calc type (mocking only run_freq_calc itself) and inspects its actual call kwargs for checkpoint/resume, plus bumped the existing count assertions to reflect the fourth call site. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…ve (AUDIT F17) run_nmr_calc() records reference_key and is_fallback_reference (which reference constants were actually used, and whether that was a substitution for an untabulated method/basis combo), but the local app's save_spectra only carried atom_symbols/shielding_iso_ppm/ chemical_shifts_ppm/reference_compound — a live CAM-B3LYP/6-31G* run correctly reports the B3LYP/6-31G* substitution, but the saved result lost both fields, so replaying it from History showed only reference_compound='TMS' with no indication a substitution ever happened. The batch NMR serializer (nmr_result_payload) already includes both. Local save now includes reference_key/is_fallback_reference too. Also surfaced the fallback explicitly on the result card itself (format_nmr_result), which previously showed the reference row identically whether it was an exact match or a substitution — a real gap independent of persistence, since the live card had the same blind spot. Added tests: a real _do_run() dispatch for NMR Shielding (mocking only run_nmr_calc) confirming save_result's spectra actually carries both fields, plus card-rendering checks for the warning appearing on a fallback and not appearing on an exact match. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
FreqResult.thermo (ZPVE, H, S, G, temperature) was computed and shown on the live Frequency card, but neither the interactive save path (app.py) nor the batch/SLURM save path (worker_payload.freq_result_payload) wrote it into the saved result.json — only frequencies, IR intensities, Raman activities, displacements, and the bare ZPVE survived a save. History had no way to show thermochemistry it once had. Fix: - app.py's local Frequency save now serializes result.thermo into spectra["ir"]["thermo"] with explicit units, temperature (K), pressure (1 atm), and the approximation model name (ideal-gas / rigid-rotor / harmonic-oscillator), or None when no thermo was computed. - worker_payload.freq_result_payload does the same for the batch/SLURM path; slurm_ingest.py and results_storage.py already pass the spectra dict through untouched, so no changes were needed there. - app_formatters.format_past_result now restores a persisted thermo block into the History card (H/S/G/ZPVE at temperature/pressure), matching the live card's rendering; older saved results with no "thermo" key render exactly as before (silent no-op). Tests: new coverage in tests/test_worker_payload.py (thermo payload present/None) and tests/test_app_formatters.py (History card restores thermo; renders nothing when absent). Full relevant suites green: tests/test_worker_payload.py, tests/test_app_formatters.py, tests/test_slurm_ingest.py, tests/test_freq_calc.py, tests/test_app.py (all passed, not network/slow). ruff + black clean. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…F rescue (AUDIT F19) The serial finite-difference displacement loop explicitly matches the reference SCF's density-fitting choice (`_try_density_fit(_mf_d, enabled=_density_fit_used)`) and threads the caller's `scf_rescue` flag through (`run_scf_with_rescue(_mf_d, dm0=_dm0, rescue=scf_rescue)`). The parallel IR worker (`freq_ir_workers.run_displaced_scf`) had no density-fitting parameter at all — every displaced SCF ran without density fitting even when the reference was fitted — and hardcoded `run_scf_with_rescue(mf, dm0=dm0)`, always taking the default `rescue=True` regardless of what the caller requested. The parallel Raman worker (`freq_raman_workers.run_displaced_polarizability`) already matched density fitting correctly but had the same rescue gap. Enabling parallel IR/Raman therefore silently changed the numerical approximation whenever the reference calculation used density fitting, and silently re-enabled SCF rescue for any run that had explicitly opted out — not just a speed difference. Fix: - `freq_ir_workers.init_worker`/`run_displaced_scf` gained `density_fit` and `scf_rescue` parameters, applied via `try_density_fit(mf, enabled=density_fit)` and `run_scf_with_rescue(mf, dm0=dm0, rescue=scf_rescue)`. Both default to the old always-off/always-on behavior for backward compatibility with any caller that only passes the original positional args. - `freq_raman_workers.init_raman_worker`/`run_displaced_polarizability` gained a `scf_rescue` parameter, threaded the same way. - `freq_calc.py` and `raman_calc.py` now pass `_density_fit_used`/ `density_fit_used` and `scf_rescue` through each `ProcessPoolExecutor` initializer's `initargs`. Tests: new regression coverage in tests/test_freq_calc.py (density_fit applied/defaults-off, scf_rescue honored, via spies on try_density_fit/run_scf_with_rescue) and tests/test_freq_raman_workers.py (scf_rescue honored/defaults-true). Full relevant suites green: tests/test_freq_calc.py, tests/test_freq_ir_workers.py, tests/test_freq_raman_workers.py, tests/test_raman_calc.py (95 passed, not network). ruff + black clean. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
pyproject.toml's [tool.setuptools] declared an explicit `packages = ["quantui", "quantui.backends"]`, omitting the real `quantui.engines` subpackage (engines/__init__.py, base.py, pyscf_engine.py, pyfock_engine.py). A wheel built from this project shipped no engine files at all; importing quantui.engines from that installed (non-editable) wheel raised ModuleNotFoundError. Every test in this suite runs against `pip install -e .`, which never consults this package list at all, so CI stayed green while a real distributed install was broken. Fix: switch to setuptools automatic package discovery (`[tool.setuptools.packages.find]`, `include = ["quantui", "quantui.*"]`, `exclude = ["tests", "tests.*"]` — tests/ has its own __init__.py, so it is a real discoverable package and must be excluded explicitly). This ships quantui.engines by construction and means the next new subpackage doesn't need a second edit here that's easy to forget. Tests: new tests/test_packaging.py (marked slow — each test builds a real wheel via the PEP 517 `build` frontend) asserts quantui.engines/ quantui.backends/bundled data files are present in the built wheel, tests/ is absent from it, and quantui.engines imports cleanly from the wheel installed into a throwaway venv and probed from outside the repo checkout — the audit's own reproduction methodology. Added `build` to the dev extra so this runs in CI. 5 passed locally (~70s); ruff + black clean; editable install (`import quantui.engines`) still works unchanged. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
… F21) _worker_command built the sbatch job's inner command line by inserting sys.executable and the request path directly into shell text, unquoted: for a path like "/tmp/audit folder/request.json" the generated command split into "--request /tmp/audit" plus a stray "folder/request.json" argument — a different, broken invocation, not a cosmetic issue. The apptainer branch quoted staging_dir and the image path but not the inner worker command it wraps. The generated #SBATCH --output/--error directive lines have the same problem one level up: sbatch tokenizes those lines the same way, so an unquoted path with a space splits there too. Fix: - _worker_command now wraps sys.executable, the request path, the apptainer image, and the staging directory in shlex.quote (or an equivalent double-quoted directive) before they ever reach the generated shell/script text, so any of them can contain a space (or any other shell metacharacter) safely. - SLURM_SCRIPT_TEMPLATE's --output/--error directives are now double- quoted for the same reason. - job_name was already sanitized to [A-Za-z0-9_-] before reaching the template, and _submit_to_slurm already calls sbatch via an argv list (no shell involved) — both unaffected, confirmed by reading rather than changed speculatively. Tests: new tests/test_backends_slurm.py::TestSlurmBackendWorkerCommandQuoting reproduces the audit's own scenario — a request/staging path containing a space — for the plain and apptainer-wrapped worker command, an end-to-end dispatch() whose submit.slurm survives a real shlex.split tokenization pass with the request path intact as one argument, and the quoted --output/--error directive lines. Full relevant suites green: tests/test_backends_slurm.py, tests/test_app_slurm.py, tests/test_slurm_ingest.py, tests/test_slurm_ingest_cl26.py, tests/test_backends_slurm_errors.py (80 passed, not network). ruff + black clean. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…heck (AUDIT F22)
test_optimizer.py's `pyscf_only` skip gate additionally required
`ase.calculators.pyscf.PySCF` — a module ASE does not ship in any
released version. quantui.optimizer implements and uses its own ASE
Calculator, `_QuantUIPySCFCalc`, and never imports
`ase.calculators.pyscf` at all. The extra check meant every
`@pyscf_only` test in this file skipped with a misleading "not
installed" reason on any environment where ASE and PySCF were both
genuinely installed and working — 16 tests, per the audit — even though
real QuantUI optimizations ran fine.
Fix: drop the `_ASE_PYSCF_AVAILABLE` probe and its `ase.calculators.pyscf`
import; `pyscf_only` now gates on `ASE_AVAILABLE and _PYSCF_AVAILABLE`
only, matching what quantui.optimizer actually needs.
Tests: removing only this obsolete condition takes the file from
(previously) 16 skipped to 43 passed / 0 skipped here — matching the
audit's own reproduction ("Removing only this obsolete marker ... made
the entire file pass: 42 passed"; this session's audit fixes added one
more test since). Added
TestPyscfOnlyGateNotObsolete::test_gate_does_not_require_ase_calculators_pyscf,
which asserts ase.calculators.pyscf is genuinely absent in this
environment yet the gate still evaluates to "available" — a regression
guard against reintroducing the same obsolete check. Full file green:
44 passed, 0 skipped (not network). ruff + black clean.
Contributions:
- Claude (Sonnet 5): code edits, review, and conceptual discussion
- Jonathan Schultz: overall vision, planning, review, and orchestration
Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
… (audit additional concerns) Several result labels/docstrings stated or implied more than the code actually computes — none change a computed number, all clarify what a displayed or documented value actually is: - **TDHF is not CIS.** tddft_calc.py and app_runflow.py repeatedly called the code's `mf.TDHF()` path "TDHF (CIS)"/"equivalent to CIS". Full TDHF (RPA) includes the excitation/de-excitation coupling that CIS (HF's Tamm-Dancoff approximation) drops — a real methodological difference, not a synonym. Labels/docstrings now say "TDHF/RPA" or "TDHF (the random-phase approximation)"; PySCF's own discussion of the distinction is linked from the module docstring. - **Alpha-channel HOMO-LUMO gap.** session_calc.py's gap extraction already read spin channel 0 only for an open-shell (UHF/UKS) reference (its own comment said so) but the result card never did. Both format_result and format_past_result now label the row "HOMO-LUMO gap (α)" for UHF/UKS/ROHF/ROKS, via a new shared `_homo_lumo_gap_label` helper. freq_calc.py's `mo_energy_hartree`/`mo_occ` fields (which feed the Energies panel's orbital diagram) get the same alpha-only documentation at their dataclass fields and extraction site. - **Post-HF dipole/Mulliken are HF-reference values.** session_calc.py extracts both from `mf` even for MP2/CCSD/CCSD(T), which never builds a correlated density; that can be a reasonable teaching simplification, but the card presented them as unqualified properties of the requested method. `_result_extra_rows` now appends "(HF reference — not a correlated MP2/CCSD property)" to both rows whenever a post-HF correlation energy is present. - **Reorganization-energy scope.** `reorg_channels_html` now states, once per card, that the shown ion multiplicity is a minimal-spin default from electron-count parity (not a ground-state determination — most relevant for transition-metal ions) and that any selected solvent applies only to the four single-point energies, not the (always gas-phase) geometry relaxations. Same two points added to reorganization_energy.py's module docstring next to `_ion_multiplicity`/`run_reorganization_energy`. Tests: full relevant suites green — tests/test_app_formatters.py, tests/test_reorg_persistence.py (65 passed), tests/test_log_utils_digest.py, tests/test_mulliken_panel.py, tests/test_session_calc.py, tests/test_worker_payload.py, tests/test_freq_calc.py, tests/test_tddft_calc.py, tests/test_app.py (460 passed, not network) — no existing test asserted the old unqualified labels/strings. ruff + black clean. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
… is immutable (audit additional concerns) Two related export/visualization completeness gaps: **Exported NPZ lacked the fields the cube helper requires.** The standalone script's `np.savez` call saved only energy/mo_energy/ mo_coeff/converged (plus the F13 post-HF fields). But `orbital_visualization.generate_cube_file` requires `mol_atom` and `mol_basis` — raising "results.npz does not contain 'mol_atom'/ 'mol_basis' keys. Re-run the calculation with the updated script template" — a template that never actually wrote them, so a cube could never be generated from a standalone-exported result. It also omits `mo_occ`, which the same function uses to infer charge/spin for charged/open-shell molecules. Fix: the template's `np.savez` call now also writes `mol_atom` (the PySCF atom string already on `mol`), `mol_basis`, and `mo_occ`. **Cube provenance could name the wrong method.** app_visualization.py's `render_orbital_isosurface` took the method label for the cube's provenance comment from the LIVE Method dropdown at Generate time — not necessarily what actually produced the stored `mo_coeff` (its own comment already acknowledged this: "not necessarily what actually produced the stored mo_coeff"). The dropdown can change, or a different History result can populate the panel, between orbitals being loaded and Generate being pressed. Fix: `show_orbital_diagram` now snapshots `result.method` into `app._last_orb_method` at the moment the orbitals are loaded from that result; `render_orbital_isosurface` reads that snapshot instead of the dropdown — immutable result provenance instead of a mutable, disconnectable UI control. Reset alongside the other `_last_orb_*` state in app_analysis.py so a context without orbitals can't leak a stale method label either. Tests: new tests/test_calculator.py::test_exported_npz_has_fields_the_cube_helper_needs executes the real generated script (subprocess) and then calls generate_cube_file on its results.npz — the actual regression: it used to unconditionally raise. New tests/test_app.py::test_render_orbital_isosurface_uses_snapshotted_method_not_live_dropdown loads orbitals from a method='B3LYP' result, changes the dropdown to 'RHF', and confirms the generated cube's method kwarg still says 'B3LYP'. Full relevant suites green: tests/test_calculator.py, tests/test_orbital_visualization.py (89 passed), tests/test_app.py (313 passed, not network). ruff + black clean. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…cation (audit additional concerns) freq_calc.py and raman_calc.py sized the opt-in parallel IR/Raman displacement pool from a bare `os.cpu_count() or 1` — the WHOLE machine's core count, regardless of any SLURM allocation or cgroup/ container CPU limit. On a 128-core host with an 8-core SLURM allocation and 18 displacements, that picks 18 workers x 7 threads each, oversubscribing the actual allocation by more than an order of magnitude. Fix: new `freq_ir_workers.available_cpu_count()`, preferring (most authoritative first) `SLURM_CPUS_PER_TASK` (authoritative even when the cluster doesn't enforce cgroup CPU limits, which many don't), then `os.sched_getaffinity(0)` (respects an enforced cgroup/container limit), then `os.cpu_count()` as the final fallback — mirroring the same precedence `quantui.utils.get_session_resources` already uses elsewhere, just adding the SLURM-specific env var this worker-sizing path was missing. freq_calc.py and raman_calc.py now call this instead of `os.cpu_count()` directly. Tests: new tests/test_freq_ir_workers.py::TestAvailableCpuCount covers SLURM_CPUS_PER_TASK precedence, invalid/non-positive value handling (falls through rather than raising), and the affinity/cpu_count fallback. Full relevant suites green: tests/test_freq_calc.py, tests/test_raman_calc.py, tests/test_freq_ir_workers.py, tests/test_freq_raman_workers.py (100 passed, not network). ruff + black clean. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…normalized broadening (audit additional concerns) **Fixed 400-4000 cm-1 range hid computed modes.** ir_plot.py/ raman_plot.py hardcoded both the plotted x-axis range AND the broadened- mode grid to a fixed 400-4000 cm-1 window. A real RHF/STO-3G water calculation has O-H stretches at 4486.7/4788.3 cm-1: in stick mode these were silently clipped off the right edge by Plotly's fixed xaxis.range; in broadened mode they never even entered the Lorentzian sum, which was only evaluated on that fixed grid - a computed mode simply invisible, not a rendering choice a user could work around. Fix: `_default_xrange` now returns [400, 4000] widened (with a 100 cm-1 margin) to also cover every real (positive) frequency actually present; `_grid_for_range` builds the broadened-mode grid from that same range instead of the old fixed one. The common case (all modes within 400-4000) renders identically to before. app_visualization.py's UV-Vis broadening already had its own dynamic per-render x-range (unaffected). **Height- vs area-normalized broadening was undocumented.** ir_plot.py/ raman_plot.py/app_visualization.py's UV-Vis broadening all use a height- normalized Lorentzian (peak height = the supplied intensity/activity/ oscillator-strength, matching the stick plot and matching Gaussian/ORCA- style displays), so the AREA under a broadened peak scales with FWHM even though its height does not. This is a deliberate, standard convention, not a bug - left unchanged - but was never stated, so a student comparing areas across different FWHM settings could draw a wrong conclusion. Documented at each broadening call site and in ir_plot.py's module docstring (the canonical explanation the other two modules point back to); no rendered numbers or default settings changed. Tests: new tests/test_ir_plot.py::TestRangeCoversRealModes and tests/test_raman_plot.py cases reproduce the water/STO-3G scenario - range widens past a mode above 4000 cm-1 or below 400 cm-1, the high-frequency stick/marker is actually plotted, and the broadened trace carries real (non-zero-baseline) signal near the high-frequency peak. Full relevant suites green: tests/test_ir_plot.py (26 passed), tests/test_raman_plot.py (8 passed), tests/test_app.py's UV-Vis/IR/Raman tests (15 passed, not network). ruff + black clean. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
`pyscf = pytest.importorskip("pyscf")` assigned a variable never used
below it (only the skip side-effect was needed) — a ruff F841 violation
introduced in this session's F02 commit that a whole-repo `ruff check`
(rather than just the touched-file checks run at the time) surfaces.
No behavior change.
Contributions:
- Claude (Sonnet 5): code edits, review, and conceptual discussion
- Jonathan Schultz: overall vision, planning, review, and orchestration
Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…st gating; satisfy mypy in ir_plot.py Three independent CI failures on PR #120, all introduced earlier in this branch: 1. **quantui/calculator.py** — the F06 fix made generate_calculation_script() call ecp_for_basis(), which imports pyscf unconditionally. Script *generation* (not just running the generated script) is meant to work on any platform per this module's own docstring ("students can download and run independently" — e.g. generate on Windows, run later on Linux/WSL or a cluster); Windows CI has no pyscf installed at all, so this broke script generation itself with ModuleNotFoundError, taking down tests/test_calculator.py's TestScriptGeneration/ TestCalculationIntegration, tests/test_phase1.py::test_calculator, and tests/test_app.py::TestExportScriptCallback with it. Fix: wrap the ecp_for_basis() call in try/except ImportError, falling back to the pre-AUDIT-F06 `ecp = {}` (all-electron) on a machine without PySCF — never worse than before that fix, and unchanged on any machine that actually has PySCF (which still gets the correct ECP mapping). 2. **tests/test_pes_scan.py** — inserting AUDIT F09's new `TestRunPesScanRejectsUnconvergedScf` class ahead of the existing `@_pyscf_available` / `@pytest.mark.slow` decorator pair left those decorators attached to the new class instead of to `TestRunPesScanIntegration`, which they were originally written for — so `TestRunPesScanIntegration` ran unconditionally, including on Windows CI (no PySCF), where it failed with ImportError instead of skipping. Restored the decorators on `TestRunPesScanIntegration`. 3. **quantui/ir_plot.py** — mypy's `no-any-return` on `_grid_for_range`: the new `_default_xrange`/`_grid_for_range` helpers (added in the spectral-range fix) took/returned a bare `list`, so `xrange[0]`/ `xrange[1]` resolved to `Any` and `np.arange(Any, Any, Any)` typed as `Any` against a declared `-> np.ndarray` return. Typed both as `List[float]`. Tests: full relevant suites green — tests/test_calculator.py (36 passed, including a new regression that monkeypatches `__import__` to simulate PySCF being absent and confirms script generation still succeeds), tests/test_phase1.py, tests/test_pes_scan.py (real PySCF integration tests still run and pass on this Linux environment, where `_pyscf_available`'s platform check is true), tests/test_app.py (313 passed), tests/test_ir_plot.py/test_raman_plot.py. mypy clean on ir_plot.py (`no-any-return` gone). ruff + black clean. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
The previous fix (typing xrange as List[float]) satisfied a locally- installed mypy 2.3.1 but not the version this project actually pins (mypy~=1.10.0, per pyproject.toml's dev extra and CI's own "mypy~=1.10.0" install) — confirmed by installing mypy==1.10.1 in an isolated venv and reproducing the exact CI failure locally. That older numpy-stub resolution for np.arange() with non-literal float bounds still infers `Any` rather than `ndarray[Any, dtype[Any]]`, regardless of the parameter's declared type. Fix: explicit `cast(np.ndarray, ...)` around the np.arange() call — the same pattern already used elsewhere in this codebase for pyscf (which has no type stubs either): document the real, known return type instead of relying on stub inference that differs across mypy/numpy versions. Verified against the actual pinned toolchain this time: mypy==1.10.1 + unpinned numpy/types-requests (matching CI's exact install command) reports zero errors on quantui/ir_plot.py and on the full quantui/ package (90 source files). ruff + black clean; tests/test_ir_plot.py and tests/test_raman_plot.py still green (28 passed). Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
test_ecp_embedded_for_heavy_element_basis (added this session for
AUDIT F06) asserts the actual resolved ECP mapping
(mol.ecp = {'Na': 'LANL2DZ'}), which requires PySCF itself
(ecp_for_basis() looks up LANL2DZ's ECP table via
pyscf.gto.basis.load_ecp) — unlike script *generation*, which the
previous commit made work without PySCF installed by falling back to
{} on ImportError. On Windows CI (no PySCF), that fallback correctly
produced mol.ecp = {}, which is not what this specific test checks for,
so it failed there instead of skipping. Added the same
`pytest.importorskip("pyscf")` guard already used by the file's other
PySCF-dependent tests.
Tests: tests/test_calculator.py — 36 passed (all PySCF-dependent tests
run and pass here, where PySCF is installed). ruff + black clean.
Contributions:
- Claude (Sonnet 5): code edits, review, and conceptual discussion
- Jonathan Schultz: overall vision, planning, review, and orchestration
Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
jonathanschultzNU
pushed a commit
that referenced
this pull request
Sep 9, 2026
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E7tooXTTNTfemGJstUib8F
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
2 participants
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.
Summary
This PR resolves 22 audit findings (F01–F22) spanning thermochemistry calculations, convergence reporting, ECP handling, solvent UI logic, packaging, and worker infrastructure. Each fix is isolated and independently testable.
Key Changes
Thermochemistry & Convergence (F01, F07, F08, F15)
cc_convergedfield;convergednow reflects both HF reference and CC solver status.Functional Resolution & Dispersion (F03, F04)
wb97xto full LibXC namehyb_gga_xc_wb97x_d(the actual Chai & Head-Gordon functional with built-in dispersion, not external D3).dispersion_appliedfield to SessionResult to track whether Grimme D3 was actually available and applied; warn in summary if D3 was requested but unavailable.ECP & Basis Handling (F05, F06, F14)
ecpdict to IR/Raman worker initializers so heavy-element ECPs (e.g., LANL2DZ on Na) are not silently dropped, causing all-electron fallback.infer_charge_and_spin()to account for ECP core electrons when inferring charge from atomic numbers; handle ROHF (1-D occupation) correctly.UI & Solvent Logic (F11)
Checkpoint & Resume (F16)
checkpointandresumearguments torun_freq_calc()in interactive Frequency runs, matching Geometry Opt / PES Scan / Reorganization Energy.Serialization & History (F12, F18)
mp2_correlation_hartree,ccsd_correlation_hartree,ccsd_t_correction_hartree,cc_converged), solvent, GPU, and density-fit metadata through staging JSON; restore them in History.Packaging & Deployment (F20)
pyproject.tomlto usefind:discovery instead of explicit package list, ensuringquantui.enginessubpackage is included in built wheels (was silently omitted before).Geometry Optimization & PES Scan (F09, F10)
Worker Infrastructure (F19, F21, F22)
density_fitandscf_rescueparameters to IR