Repository navigation
Fix the Low findings from the 2026-11-10 audit - #172
Merged
Merged
Conversation
- freq_calc: the "SCF converged. Computing analytical Hessian..." status was emitted before mf.converged was read, so an unconverged reference SCF was announced as converged. Read converged first and word the status accordingly. - session_calc: HARTREE_TO_EV comment claimed consistency with PySCF; it is CODATA 2018, PySCF's nist.HARTREE2EV is CODATA 2014 (negligible). - vib_cache: module docstring index.json example now shows fps, bgcolor and the per-mode mac the code actually writes. The wB97X-D "wb97x + external D3" comments named in the audit were already corrected by the SCF-2 fix on main. Text/comment-only; no test. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…spin (audit SCF-12) - engines.run_calc matched methods case-insensitively but the basis by exact string, so "sto-3g", "def2-svp" or the Pople parenthesis notation "6-31G(d)" (same set as "6-31G*") were refused. A new _match_basis compares case-insensitively and accepts either Pople notation (config.pople_notation_alias); the request handed to the engine carries the engine's own listed spelling, so adapters that compare exactly (PyFock) still accept it. - PyFock: def2 basis sets put an ECP on every element after Kr (Z > 36); PyFock applies none, so such molecules would run all-electron against a valence-only basis. _validate_request now refuses them with a message pointing at PySCF. - PyFock single-point SessionResult left multiplicity=None; it is now set from the request (always 1 on this engine). Not changed (needs a PyFock install to verify): the process-global stdout/stderr redirect and the coordinate-unit check. Tests: test_engines_registry run_calc basis matching (parametrised) and unknown-basis rejection; test_pyfock_engine ECP rejection, the Z > 36 helper, and multiplicity on the native result. All new tests fail on the old code. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…b index (audit SCF-13) - QUANTUI_DISABLE_GPU accepted only 1/true/True in gpu_offload and pyfock_gpu, while freq_ir_workers._truthy (which claims to match it) accepts 1/true/yes/on in any case. New gpu_offload.env_gpu_disabled() uses the _truthy convention; both GPU probes call it. - session_calc: the run-body closure _to_numpy_array duplicated _host_array line for line; removed and the gap extraction now calls _host_array. The existing regression test that pinned the closure's name (test_bug_regressions_2026_05_25) now exercises _host_array and checks the gap extraction uses it. - vib_cache.save_cached_html: the index read-modify-write is now under an in-process lock, so concurrent saves of different modes can't drop each other's manifest entry. - scf_robust: the bootstrap SCF now inherits the target's per-cycle callback, so a Cancel click also stops the bootstrap stage. Not done (design/perf, out of scope for a Low fix): merging the IR and Raman displaced-SCF loops; skipping the bootstrap when the target is itself plain RHF/UHF. Tests: test_gpu_offload env_gpu_disabled true/false values, test_pyfock_gpu opt-out with "yes", test_vib_cache concurrent saves, test_scf_robust bootstrap callback. The pyfock_gpu, vib_cache and scf_robust tests fail on the old code. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…t GEOM-8) GEOM-8 (the fallback deleted frame irc_steps + 1, which is a real descent point whenever the steepest descent took steps, and kept BFGS's repeated start) was already fixed on main by 5ec16f4 (GEOM-1): the index is now taken just before BFGS. The existing endpoint tests only cover the case where the descent converges on its own, so BFGS never runs. This adds the missing case: descent capped at three accepted steps, BFGS finishes; asserts no repeated frame and that each descent step sits at its expected trajectory index. Fails when the deletion index is taken before the descent (the GEOM-8 shape). No code change. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…pped" (audit GEOM-9) - classify_stationary_point counted only imaginary values above 50 cm-1, so a real soft TS (torsional barriers, 20-50i) with no larger mode was reported "Not a transition state ... ended at a minimum". With no large imaginary value but at least one small one, the verdict is now "Inconclusive": soft TS or numerical noise, animate the mode and tighten convergence. n_imaginary is unchanged (0), so nothing that keys on the count changes. The TS help text lists the new verdict. - apply_frequency_check left the placeholder "Frequency check skipped." when the check ran but returned no frequencies; it now says the check ran and produced no vibrational frequencies (prefixed with the not-converged note when the search did not converge). Tests: test_ts_search TestVerdict small-only-imaginary verdict and the empty-frequencies verdict (run_freq_calc stubbed). Both fail on the old code. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…M-10) scf.RHF on an open-shell molecule silently becomes ROHF, which PySCF cannot use for TDHF (TD-DFT failed: "'NoneType' object is not callable", empty spectrum) or for the analytic Hessian the TS search and IRC need (NotImplementedError). The reorganization-energy workflow already promoted RHF/HF to UHF for open-shell species (_promote_method); the other three paths did not. New reorganization_energy.promote_open_shell_hf wraps _promote_method and writes a one-line note to the progress stream when it promotes. run_tddft_calc (SCF body), run_ts_search and run_irc call it before building the SCF / calculator, so the run and the result's method label are UHF. Tests: tests/test_open_shell_rhf_promotion.py: helper cases; TS search and IRC hand "UHF" to the calculator (stubbed); real OH radical RHF/6-31G TD-DFT runs as UHF with converged roots. The three path tests fail on the old code. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…audit GEOM-11) Each channel's lambda uses both the neutral and the ion geometry, but ReorgChannelResult.converged was taken from the ion optimization alone, so a channel built on an unconverged neutral geometry read "converged" (only the overall flag caught it). The channel flag now requires both. Test: test_reorganization_energy test_channel_converged_includes_the_neutral_optimization (optimizer and single points stubbed); fails on the old code. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…nge label (audit GEOM-11) - run_pes_scan built the shared calculator without a progress stream, so its D3-missing and SCF-rescue notices never reached the scan log (and its status heartbeat said "Optimizing geometry"). It now gets the scan's stream and status_label="PES scan". - A descending log-spaced bond range (start > stop) was silently reversed into an ascending one, both in run_pes_scan and in pes_scan_ui.build_scan_grid, so the scan ran the other way. np.geomspace(start, stop) keeps the requested direction (same points). - PESScanResult.summary() labelled max - min of the scan "Barrier height"; it is only a barrier for a scan that runs from a minimum over the barrier top. Relabelled "Energy range", as the result card already says. - Removed pes_scan_ui._clamp_selection (no callers). Tests: test_pes_scan stream handed to the calculator (stubbed) and a real H2 RHF/STO-3G descending log scan (direction + summary label); test_pes_scan_ui descending log grid. All three fail on the old code. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…it GEOM-11) - A negative frozen-atom index failed the range check but the message named the largest (valid) index instead: frozen_atoms=[-1, 1] on water said "Frozen atom 2 does not exist". It now names the offending index. - _QuantUIPySCFCalc wrote the "GPU offload active" banner on every force evaluation (it is rebuilt per geometry); it is now written once per run, when the calculator first offloads. Tests: test_structure_edit test_bad_frozen_lists (negative index case); test_optimizer test_gpu_banner_is_written_once_per_run (try_to_gpu stubbed to report a GPU, H2 RHF/STO-3G). Both fail on the old code. The banner itself still needs a look on a real GPU host (local/H200). Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…s (audit HPC-14) The batch worker diverged from the interactive run: - Geometry Opt ran the app's required final single point only with a solvent; gas-phase batch optimizations kept the optimizer's last-step SCF data and never got NPA charges / bond orders. - A "geometry optimization before the calculation" for Frequency, TD-DFT, NMR and PES Scan skipped the required single point the app runs as a convergence gate (on failure the app abandons the pre-opt and runs at the input geometry). - A worker failure logged str(exc) only; no traceback anywhere. - elapsed_s and warnings lived only on the in-memory CalculationResult and were never written to result.json. Fix: a shared _run_required_single_point helper (run_in_session with the request's solvent and scf_rescue; raises if unconverged). Geometry Opt always runs it and folds the same fields as app.py (energy, MOs, pyscf_mol_atom/basis, atom symbols, Mulliken/NPA charges, bond orders, dipole). _maybe_run_preopt runs it for every calc type except single_point, before caching the pre-opt geometry, inside the existing fall-back-to-input-geometry try block. The worker boundary appends traceback.format_exc() to live.log, and result.json gains elapsed_s and warnings. Still differs (not changed): the worker ignores the quantum_engine preference (it is not carried in the batch request; plumbing it is a design change) and calls PySCF directly rather than via engines.run_calc. Tests: TestInteractiveParity in tests/test_backends_worker.py (5 of 6 fail on the old worker). Optimizer-only mocks in test_backends_worker and test_freq_saddle_retry now stub run_in_session via an autouse fixture so they stay PySCF-free (Windows CI). Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…-15) Jobs request 4-128 GB, but nothing told PySCF: every batch calculation ran with PySCF's 4000 MB max_memory default, falling back to out-of-core paths on big-memory jobs. The shared SLURM_SCRIPT_TEMPLATE (used by SlurmBackend and the quantui-batch launcher) now exports PYSCF_MAX_MEMORY = 80% of the allocation in MB. It reads Slurm's SLURM_MEM_PER_NODE at run time, so a --mem given on the sbatch command line (rerun --more-memory) is honoured, and falls back to the #SBATCH --mem value when that variable is unset or not plain MB. apptainer exec passes the host environment through, as it already does for OMP_NUM_THREADS. Test: test_pyscf_max_memory_follows_the_allocation in tests/test_backends_slurm.py runs `bash -n` on the generated script and executes the memory block with and without SLURM_MEM_PER_NODE (fails on the old template). Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…le sbatch (audit HPC-16)
- quantui.def %environment exported "/opt/quantui:${PYTHONPATH}", which
with PYTHONPATH unset ends in ":" -- an empty sys.path entry, i.e. the
current directory, so a json.py in a student's folder shadowed the
standard library. Now "/opt/quantui${PYTHONPATH:+:${PYTHONPATH}}".
- The image installed voila unpinned and QuantUI as .[pyscf,ts], so
pyproject's voila>=0.5,<0.6 (and ipykernel) bounds never applied. Voila
is dropped from the unpinned pip line and QuantUI installs
.[pyscf,ts,app].
- The build now records /opt/build-info (pip freeze, conda list, QuantUI
version, build date), as quantui-gpu.def already does.
- apptainer/slurm/quantui-batch.sbatch had drifted from the generated
submit.slurm (no attempt dir, --nv forced, results dir override) and
is removed; both READMEs point at the generated per-job script instead.
Not changed: base images stay pinned by tag (digest pinning needs the
registry), conda packages stay unpinned, and the worker's apptainer exec
gets no --cleanenv / --env PYTHONPATH= (cannot be verified without
Apptainer; --cleanenv would also drop OMP_NUM_THREADS and
PYSCF_MAX_MEMORY). The image itself was not built here.
Tests: TestCpuImageHygiene in tests/test_cpu_container_def.py (text
checks; 3 of 4 fail on the old def, the 4th on the old tree). Extracted
%environment/%post/%runscript/%test of both defs pass `bash -n`.
Contributions:
- Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
… HPC-18) - launch-app.bat passed "%SIFDIR%" (ending in "\", which escapes the closing quote) to wslpath; the folder is now normalised without the trailing backslash first. - launch-app/dev/native.bat ran bash -c "cd '%WSLPATH%' && ...", which broke on an apostrophe in the folder path; they now use wsl --cd, as launch-native-jupyter.bat already does. - launch-native.command, launch-native.sh and launch-native-jupyter.sh source conda.sh / conda activate under set -u, whose activation scripts read unset variables; nounset is now off for those two lines. - launch-native.command listed /opt/miniconda3/condabin/conda (the conda executable, not conda.sh) as a file to source; dropped, and the Intel Homebrew miniforge prefix added. Its `open` gets `|| true` under set -e. - Fixed ports + a blind sleep opened whatever already served on 8866/8867. Every Voila launcher now refuses to start when the port is already answering, and waits for the port to answer (stopping early if Voila exited, .sh/.command) instead of sleeping a fixed time. Not changed: `wsl -d Ubuntu` stays hard-coded (choosing a distro is a decision); the Windows and macOS launchers were not run here. Tests: tests/test_launcher_scripts.py runs launch-native.sh and launch-native.command against fake conda/voila in a repo copy whose path contains an apostrophe: busy port refused, conda activation survives set -u, a Voila that dies at startup is reported (fails on the old scripts). All .sh/.command pass `bash -n`. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…C-9) `--partition` and QUANTUI_SLURM_PARTITION were pasted onto the `#SBATCH --partition=` line unchecked, so a newline in the value added #SBATCH directives or shell lines to the batch script, bypassing validate_resources(). Account/QOS/GRES were already restricted to [A-Za-z0-9_.:,=+-]. Add cluster_config.validate_partition() using the same pattern; SlurmBackend._create_job() checks the partition together with the site directives (SecurityError, before the job folder exists), and the quantui-batch launcher's partition() port raises InputError, also before its job folder is made. Tests: tests/test_hpc_audit_low.py::TestPartitionValidation (package, `quantui submit --prepare-only`, launcher). Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
`quantui submit` with several input files waits out the submit cooldown between them, but each dispatch still checks the concurrent-job limit (QUANTUI_MAX_CONCURRENT_JOBS, default 2), so everything past the second input was rejected. The code comment even cited a 30-job batch as the motivating case. Chaining the inputs would need a waiting/queueing loop, so document the limit instead: `quantui submit --help` gets a description naming both QUANTUI_MAX_CONCURRENT_JOBS and QUANTUI_SLURM_SUBMIT_COOLDOWN_S and the two ways round the limit (raise it for the command, or --prepare-only + sbatch); docs/CLI.md says the same in the submit section and lists both variables in its environment table. The misleading comment is reworded. Docs/help text only; no test. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
`quantui submit` and `quantui-batch submit` accepted input that then failed
or was silently ignored on the compute node:
- the basis was never looked at (`--basis not-a-basis` prepared, exit 0);
- unknown `--option` keys were stored and ignored, and `scf_rescue=no`
arrived as the string "no", which the worker's bool() reads as True;
- `--cores 0` / `--memory-gb 0` fell back to the estimate (`or`);
- request JSON skipped the calc-type, atom/coordinate and
charge/multiplicity checks an .xyz input gets;
- a JSON request_id with `/` crashed with a traceback in registry.save;
an .xyz whose stem contains `..` was refused as a path component;
- `quantui submit --preopt` help named three calc types; it applies to five.
Fix, in batch_input and its port in the launcher (kept equal; the launcher
gets the option/basis tables through site_constants):
- check_basis(): normalise the spelling to QuantUI's list (incl. the
6-31G(d) spelling) and warn, not refuse, for anything else - the same
policy as calculator.py, since PySCF knows many more bases;
- check_options(): yes/no/on/off/true/false/1/0 -> bool for scf_rescue and
preopt_before_run (anything else refused); warn about keys the worker
does not read for that calc type (CALC_OPTION_KEYS, from worker.py);
- check_json_request(): request_id must match [A-Za-z0-9_][A-Za-z0-9_.-]{0,63}
without "..", known calc_type, atoms/coords as Molecule() checks them,
and the charge/multiplicity the worker will use (molecule first) must
fit the electron count;
- resource overrides use `is None`, so 0 is refused by the range check;
- file labels collapse runs of dots;
- --preopt help lists all PREOPT_CALC_TYPES.
Tests: tests/test_hpc_audit_low.py::TestBatchInputChecks (package and
launcher side by side).
Contributions:
- Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…dit HPC-12) `quantui-batch rerun --more-memory` doubled the memory with no ceiling and `--more-time` past the last walltime option doubled the hours (48 h -> 96 h), so repeated reruns asked for far more than any submission may. --more-memory is now clamped to the per-job memory limit the launcher checks submissions against (SITE max_memory_gb) and refuses when the job already had that much; --more-time moves to the next longer allowed time limit (also from a hand-set value between options) and refuses past the longest. Explicit sbatch flags still pass through unchanged. The cluster_config comment called these "hard limits"; they are client-side defaults read from the user's own environment, so it now says so and points real enforcement at Slurm QOS/partition limits. (apptainer/slurm/README.md has the same wording; not touched here.) Tests: tests/test_batch_submit.py::TestRerunMore - the existing --more-time test expected 96:00:00 (the old uncapped behaviour) and now expects a refusal; new test_more_memory_stops_at_the_site_limit. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…t HPC-13) format_error_html() pasted Slurm stderr / exception text (translated category, summary, advice, raw excerpt, or the untranslated first 500 characters) into HTML unescaped, and the Cluster Jobs table and the startup "job still active" banner did the same with record fields (status, SLURM id, method/basis, calc type, created, request id). Those come from the job record JSON and request files, so a "<" in them broke or injected markup. (The pending-jobs list and the error cell were already escaped.) Escape every interpolated value with html.escape(); the static markup is unchanged. Tests: tests/test_hpc_audit_low.py::TestSlurmHtmlEscaping. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…it HPC-17) The mock in testing/mock-slurm hid only COMPLETED jobs from squeue (a cancelled or failed job stayed "in the queue" forever), returned 0 from scancel for a job id it did not know, checked the core limit against --ntasks although QuantUI's script asks for 1 task x --cpus-per-task cores, and never checked that the generated script parses. The .sh variants were separate copies that had drifted (sacct.sh still printed a bare CANCELLED). mock_slurm_cli.py now: hides every terminal state from squeue; fails scancel for an unknown id with Slurm's message; counts cores as ntasks x cpus-per-task; runs `bash -n` on the submitted script (when bash exists) and refuses one that does not parse. The .sh shims exec the Python mock, like the .cmd shims, so there is one implementation. The unused walltime parse (a ruff F841) is dropped. The existing SLURM backend/app tests pass unchanged against the stricter mock. Tests: tests/test_hpc_audit_low.py::TestMockSlurm. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
… HPC-19)
- `quantui-batch cancel` and the "still running" check in `rerun` knew
only the ids in .quantui-batch-jobs, so a job queued by the app (or a
bare sbatch) could not be cancelled from the terminal and could be
rerun while it was still running. New all_job_ids(): the log, plus the
ids in attempt-NN_job<id> folder names, plus queue entries whose script
is this folder's submit.slurm (a job that has not started yet).
`rerun --more-memory` falls back to the newest attempt's id for its
sacct lookup when there is no log.
- The job record was written to jobs_root()/<request_id>.json with no
check; record_path() now refuses an id that is not
[A-Za-z0-9_][A-Za-z0-9_.-]{0,63} or contains "..", like the
registry's safe_join. An InputError from prepare_job() (this, or a bad
site/partition setting) is reported per input instead of a traceback.
- FAILED_STATES gains PREEMPTED, BOOT_FAIL and DEADLINE, so such a run
no longer counts as an existing duplicate of a new submission.
Tests: tests/test_hpc_audit_low.py::TestLauncherSmallIssues.
Contributions:
- Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…PC-21) The SLURM monitor re-read the whole live.log every 2 s (and tracked the offset in characters), and slurm_backend_for_app() built a new SlurmBackend on every call, so its squeue/sacct status cache started empty for each Cluster Jobs refresh. - _read_new_log_text(): seek to the byte offset and read only the new bytes; an incomplete UTF-8 character at the end waits for the next poll; a file shorter than the offset (a new attempt's log) is read from the start. - slurm_backend_for_app() keeps one backend on app._slurm_backend while the app's registry is unchanged. Not done: moving cancel()'s up-to-30 s confirmation wait off the kernel main thread. cancel_slurm_run() returns the outcome synchronously to QuantUIApp's Cancel handler, which sets the button state from it, so threading it is a UI-flow change, not a small fix. Tests: tests/test_hpc_audit_low.py::TestPollingCost. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
JobRegistry and the quantui-batch launcher created the jobs/staging roots and every job folder with the default mode, so on shared scratch with umask 022 other users could list and read a student's inputs, logs and results. New folders are now created with mode 0o700 (JOB_DIR_MODE): the registry's jobs and staging roots, new job dirs and legacy per-request dirs; in the launcher, the staging root (created before the job dir, since mkdir(parents=True) would give it the default mode), job dirs and the jobs root. An existing folder keeps its mode, so a root a course has deliberately shared is left alone. Attempt folders are created by the batch script inside the job dir and are covered by its mode. Tests: tests/test_hpc_audit_low.py::TestJobDirMode. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
slurm_backend_for_app() returned the attribute read with getattr(), which mypy (as run in CI) reports as returning Any | None. cast() it; no behaviour change. Covered by TestPollingCost.test_backend_is_reused_per_app. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
Local sessions reach the private planning repo through a `.plans` symlink in the repo root (CLAUDE.md, DEV-ENVIRONMENT), but nothing ignored it, so a `git add -A` would commit the symlink into this public repo. Add `.plans` to .gitignore. The other INFRA-20 items are deliberately left alone: the merge-commit email is a GitHub account setting (no history rewrite), and moving AUDIT_FINDINGS.md is a planning-repo decision. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…t INFRA-11) The cloud hook ran bare `python -m pip install -e ".[...]"`. On the current cloud image `python` is 3.13, outside the supported 3.9-3.11, where pyscf-properties does not build, so sessions started with nothing installed and the env had to be built by hand with `uv venv -p python3.11`. The hook now checks the default interpreter's version; when it is outside 3.9-3.11 it reuses a supported .venv or creates one (`uv venv --seed -p python3.11`, else `python3.11 -m venv`), runs every pip step through that interpreter, and exports VIRTUAL_ENV/PATH via $CLAUDE_ENV_FILE so later Bash calls use it. Behaviour is unchanged when `python` is already supported. CLAUDE.md's description of the hook is updated to match. Only syntax-checked (`bash -n`); the hook was not executed here. Test: tests/test_session_start_hook.py (bash -n, every pip call uses the selected interpreter, the venv/env-file path exists, and the version gate accepts 3.9/3.11 and rejects 3.8/3.12/3.13 via a stub interpreter). 7 of 8 fail against the old hook. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
The conda env file had drifted from pyproject.toml: no pytest-xdist although pyproject's addopts carries -n=auto (a bare `pytest` in a fresh env failed), mypy as an open `>=1.0.0` floor instead of CI's ~=1.10.0, voila uncapped against the [app] extra's <0.6, build and types-requests missing, and none of the runtime MAJOR caps (jupyter<2, numpy<3, ...). The env file now carries the same ranges: runtime caps, pytest-xdist, mypy>=1.10.0,<1.11, types-requests, python-build (conda-forge name of PyPI `build`), setuptools>=70.1 (the offline packaging test needs built-in bdist_wheel), pre-commit>=3.7.0, voila>=0.5.0,<0.6 and pyscf-dispersion>=1.0. Kept as an explicit conda list rather than switching to the pyproject extras because the conda env deliberately installs rdkit/xtb-python/pyscf from conda-forge. Test: tests/test_environment_yml.py parses both files (line-based YAML, tomllib/tomli) and checks every [dev] tool is present, voila/mypy are pinned like pyproject, and for each package in both files the env range is no wider than pyproject's (probed around every bound). All three tests fail against the old env file. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…t INFRA-16) QuantUI vendors 3Dmol.js 2.5.4 (viz_assets.THREEDMOL_VERSION) and serves it in place of the CDN build py3Dmol's view(js=...) default targets. The py3Dmol-generated JS calls that build's API, but nothing checked the two stayed compatible: py3Dmol 2.5.5 (the current resolve) targets 3Dmol.js 2.5.5, and `py3Dmol>=2.0.0,<3` would let a 2.6/2.x py3Dmol targeting a newer API in silently. Cap py3Dmol at <2.6 (the vendored minor) in pyproject and local-setup/environment.yml, and document the tolerated patch skew in viz_assets.py and data/js/PROVENANCE.md. Not re-vendored (no network in this session); 2.5.4 -> 2.5.5 is patch-level. Test: tests/test_viz_assets_version.py — py3Dmol's default js URL targets the vendored major.minor; pyproject's cap admits the vendored version but not the next minor (fails on the old `<3` cap); CDN_URL names the vendored version. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…udit INFRA-17)
- `license = { file = "LICENSE" }` is deprecated (setuptools warns builds
break after 2027-02-18). Switch to the SPDX `license = "MIT"` plus
`license-files = ["LICENSE"]`, raise the build requirement to
setuptools>=77 (and the dev extra / env file, which the offline
--no-isolation packaging test builds with), and drop the licence
classifier, which setuptools >= 77 rejects alongside an expression.
Builds offline with the installed setuptools 84.2.1, no deprecation
warning; METADATA now carries License-Expression: MIT.
- data/js/PROVENANCE.md and data/library/QM9-PROVENANCE.md were shipped in
neither wheel nor sdist: add `data/**/*.md` to package-data (both now in
both artifacts).
- rdkit (pubchem, preopt, structure_upload, app_exports) and psutil
(log_utils, utils) are imported directly but only arrived transitively
(plotlymol / ipython). Declare `rdkit>=2022.3.1` (plotlymol's floor;
calendar-versioned, so no major cap) and `psutil>=5.0,<8` as runtime
dependencies; psutil added to the conda env file.
- Major caps per the existing pyproject policy on the uncapped extras:
pyscf-dispersion <2, xtb <23, sella <3 (installed: 1.7.0, 22.1, 2.6.0);
env file follows. pyscf-properties left as is (only 0.1.0 exists; the env
file installs it from git).
Tests: tests/test_packaging.py gains
TestWheelContents::test_launcher_manifests_and_licence_files_are_shipped
(launcher, every manifest, 3Dmol licence files, provenance notes) and
::test_metadata_licence_and_direct_dependencies (License-Expression /
License-File / licenses/LICENSE, rdkit + psutil in Requires-Dist). Both
fail against the old pyproject.
Contributions:
- Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
- .github/copilot-instructions.md: the transition_state entry of the _PANEL_REGISTRY sketch and the panel table omitted PES Scan (_pop_ts_guess_profile, reactant + product searches); the two tables split by a stray "Source of truth:" line (panel list, dependency groups) had an orphan header row that broke them; constraint 8 now states the inline-plotly.js rule exactly as CLAUDE.md / amended DEC-010 do (only standalone exports and plotly_embed's no-app fallback); install lines use CI's extras incl. xtb and ts (without them the GFN-FF and Sella tests skip silently); dependency table updated for the INFRA-16/17 changes (py3Dmol<2.6, rdkit, psutil, xtb/sella caps, setuptools>=77, mypy~=1.10.0); test count/baseline refreshed. - README.md "Running tests": installed only ".[dev]" and then ran "All tests" with no PySCF, so the numerical suite skipped; now installs CI's extras and says so. Test count refreshed. - SECURITY.md: "no persistent user data" was false. Scope now lists what persists (results, ~/.quantui logs, settings, checkpoints, SLURM jobs/staging, launcher notebook), the app-access model after HPC-8 (loopback bind + per-run token, QUANTUI_APP_TOKEN, token visible in `ps` for standalone Voila, JupyterLab mode), and the untrusted-input surfaces it omitted: Viewer .zip upload, structure-file upload and SLURM job-folder/result ingestion. - CLAUDE.md: test baseline ~4120 passed / 24 skipped -> ~4680 / 11 (latest known figure; not re-run here). Docs only; no tests (tests/test_docs_snippets.py and tests/test_code_quality.py still pass). Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…s (audit INFRA-12) Closes four of the CI gating gaps the audit listed: - network-marked tests (live PubChem, wheel install from PyPI) were deselected by every ci.yml job and ran nowhere. New .github/workflows/network-tests.yml runs `pytest -m network` weekly (Mon 06:41 UTC) and on workflow_dispatch; it does not gate PRs. - `-n=auto` started one xdist worker per core and each worker's NumPy/PySCF one OpenMP thread per core (N x N threads). tests/conftest.py now sets OMP_NUM_THREADS=1 (setdefault) in xdist workers before anything imports NumPy; -n0 runs and an explicit caller value are untouched. - pages.yml granted pages: write + id-token: write workflow-wide, so the build job (third-party build tooling) inherited them. Top level is now contents: read; only the deploy job gets the two write permissions. - The Windows job --ignore'd test_session_calc.py, test_optimizer.py and test_preopt.py although all three have skip guards. Verified locally by running them with pyscf/xtb/sella blocked (sys.modules[...] = None): one test, TestPyscfOnlyGateNotObsolete, asserted PySCF was installed without a guard; it now skips without ASE+PySCF. After that the three files give 75 passed / 46 skipped with PySCF blocked, so the --ignores are removed. Not done here (left for the instructor): a macOS job and a coverage floor. Tutorial notebooks under nbmake were not wired in: nbmake is not installed in the dev env, so they could not be checked locally. Test: tests/test_ci_gating.py (scheduled -m network workflow exists, Pages write perms job-scoped, no --ignore in the Windows run line, xdist OMP cap). All five fail on the previous workflow/conftest files. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…figure (audit VIZ-11) The IR, Raman, UV-Vis, ECD and NMR update paths, the orbital energy diagram and the PES profile caught plotting exceptions, logged them and nulled the stored figure, but left the Output widget untouched. The previous molecule's (or previous setting's) plot then stayed on screen as if it belonged to the current result. New _show_plot_error(app, out, what, exc) swaps an inline, HTML-escaped "<what> could not be plotted: <error>" message into that output. Used in each of those except blocks; for the orbital diagram only when the matplotlib fallback also fails (reporting the original Plotly error). The live Y-range/core toggles of the orbital diagram are unchanged (same result, so the figure on screen is not stale). Tests: tests/test_viz_low_audit.py::TestFailedPlotsReplaceTheStaleFigure (IR with an escaped message, NMR, PES). Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…-12) Several error and status messages interpolated exception text, file paths or a formula straight into HTML. Exception text can echo file contents and a formula can come from a saved result, so markup in either was rendered (or broke the message). Now escaped: - app_visualization: trajectory render/viewer failures, the trajectory export "Saved: <path>" and "Export failed" lines, frame render failure, the isosurface failure message, the density/ESP _show_msg helper and _vib_err (both plain-text helpers now escape; the one _vib_err caller that passed <code> markup now passes plain text). - visualization_py3dmol.render_molecule_html: "Visualization failed". - viz_assets.standalone_html: the <title> of exported files. - orbital_visualization.orbital_summary_html: the formula. Not repeated: the VIZ-1 fix (6404d2e) already escaped symmetry labels and the script payloads; downloads.download_link_html already escapes names. show_opt_trajectory has a local `html` string that shadows the module, so its three messages use a module-level `_escape` alias instead. Tests: tests/test_viz_low_audit.py::TestErrorTextIsEscaped (vib error, export title, orbital summary formula, molecule viewer failure, and the trajectory viewer failure inside show_opt_trajectory). Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…s (audit VIZ-13) - smoke_launch.py fetched http://127.0.0.1:<port> without the per-run token. Voila answers that with its login page (HTTP 200), so the check only proved a server was listening. It now reads the tokenised URL that `quantui view` prints ("Starting QuantUI Viewer at ...?token=..."), fetches it (no proxy, 120 s read timeout because Voila executes the notebook before the page completes; overall deadline 120 -> 180 s) and requires a rendered widget (nbconvert's lab template emits each widget output as <script type="application/vnd.jupyter.widget-view+json">). On failure it prints the start of the page. The launcher's stdout is echoed and drained by a thread so Voila never blocks on the pipe. - viewer-builds.yml: the release job writes SHA256SUMS.txt for the installers and uploads it with them; README says how to check it. - packaging/viewer/README.md still said isosurfaces are unavailable in the Viewer. Since VW.7 the HOMO/LUMO cubes saved with a result are drawn; the table row and Known gaps now say so. Not changed (need release access, network or a policy call): pinning actions/miniforge/constructor/wheel versions, signing, --clobber. Tested locally against a fake `quantui` that prints the URL and serves a page: passes with the widget marker, fails (rc 1) on a login-style page. Not run against real Voila here (not installed); the viewer-builds workflow is the real check. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…history method, dead code (audit VIZ-14) - nmr_plot.plot_nmr_spectrum drew δ ascending left to right; NMR spectra are read high -> low. New descending=True (default) reverses the axis, like the IR/Raman plots' descending option. tests/test_nmr_plot.py's 13C window test now checks max(range) instead of range[1]. - The 0.05 ppm "equivalent nuclei" merge window was applied to 13C too, where symmetry-equivalent carbons in an unsymmetrised geometry differ by more. Per-nucleus windows: 1H 0.05 ppm (unchanged), 13C 0.5 ppm (scaled to 13C's 0.1 ppm reporting precision, as 0.05 is to 1H's 0.01). The 13C value is a judgement call - flagged for review. - visualize_molecule(backend="auto") picked PlotlyMol whenever installed, while its docstring and info box say py3Dmol. Now py3Dmol first. The app always passes a concrete backend, so the app is unaffected; the PlotlyMol integration script now asks for plotlymol explicitly. - load_frontier_cube: a non-numeric "index" in frontier_cubes/index.json raised an uncaught ValueError/TypeError; the entry is now skipped. - History-loaded orbitals had no method, so a cube generated from a History result omitted it from its provenance line; pop_energies now copies the history context's method onto the stub. - Removed dead code: app_visualization.traj_step_html and render_traj_frame (and their QuantUIApp wrappers), and visualization_py3dmol.get_installation_message. No callers. Not changed: colour-by-charge forcing ball+stick (populations_overlay) needs the current viewer style plumbed through - a design choice; the Plotly isosurface branches are kept deliberately (see the VIB_EXPORT docstring). Tests: tests/test_viz_low_audit.py::TestNmrPlotConventions and ::TestVizNits. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
refresh_comparison() called plot_comparison() and dropped the returned matplotlib Figure: nothing was displayed (no inline backend under Voila) and every "Add to comparison" leaked one open figure. on_compare already rendered the chart correctly as an inline PNG. Both paths now use one helper, _comparison_chart_html(), which renders the figure to a base64 PNG <img> and always closes the figure. Test: tests/test_appui_audit_low.py test_in_session_comparison_shows_chart_and_closes_figure (fails on the old code) and test_saved_result_comparison_still_shows_chart. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
- on_calc_type_changed built new HBox/HTML wrappers (plus Layout/Style models) on every calc-type switch and never closed them. The previous wrappers are now closed after the switch; persistent app widgets are never closed. - _calc_type_badge and the History calc-type chips lacked Transition State and IRC, so those results showed a raw key and vanished whenever any chip was on (also listed under UI-13). - Files tab: the default selection came from iterating a set (arbitrary per process); it is now the first entry as listed. - Clipboard helpers claimed "copied" although the kernel cannot see whether the browser allowed the write; the wording now says so, and the paths are HTML-escaped. - The ImportError card told every platform "On Windows: use the Apptainer container"; the hint is now platform-specific. - A Cancel that arrived as the calculation failed made the error handler's log.write re-raise _CalcCancelled: no error card, status stuck on "Cancelling...". The error handlers clear the flag first. - Arming Exit while a calculation (or calibration) runs now warns that it will be stopped and its result lost. - Dead code removed (each grepped: no callers): _set_molecule_threadsafe, _set_molecule_state_only, _set_plotly_figure_output, _render_plotly_figure, _traj_step_html, _render_traj_frame, the no-op _install_run_output_scroll_guard (and its flag), the unread _preopt_available attribute, and the unused _on_freq_seed_changed / _on_tddft_seed_changed aliases. Two tests in tests/test_app.py that only asserted those attributes exist are removed. Not changed: history refresh cost (no obviously safe cache: shared mutable result dicts and patched load_result in tests), n_cores=1 (the estimator normalises by the same 1 on both sides), the notebook skip-execution tag (needs a Voila check), _perform_exit signalling the parent server (design decision). Tests: tests/test_appui_audit_low.py (leak, cancel-during-error, badges/ chips, Files default, dependency hint) and tests/test_exit_confirm.py::test_arming_while_a_calculation_runs_warns_it_will_be_lost. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
pop_pes_plot and pop_irc_profile took min() over the saved energies. A failed scan point is saved as NaN, and when it is the first element min() returns NaN, so every relative energy became NaN and the reloaded profile was blank. The live path (pes_scan.py) already used a finite-only minimum. Both functions now take the minimum over finite energies only (a null entry is treated as NaN too); a profile with no finite point is reported as not available. Tests: tests/test_appui_audit_low.py test_reloaded_pes_scan_with_failed_first_point_is_plotted (NaN and null) and test_reloaded_irc_with_failed_first_point_is_plotted (fail on the old code). Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
_panel_unavailable_html HTML-escapes its message, so the Frequency "No optimization steps in this run. Tick <b>Geometry optimization before calculation</b> ..." hint rendered the tags literally. The message is now plain text with the checkbox label in quotes; the escaping helper stays. Test: tests/test_appui_audit_low.py test_unavailable_panel_messages_show_no_escaped_markup (every calc type; fails on the old code for frequency). Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
- METHOD_INFO["PBE-D3"] said D3BJ; the code applies zero-damping D3
(session_calc._D3_VERSION = "d3zero", as Gaussian's GD3).
- METHOD_INFO["CCSD"] called CCSD the "gold standard"; that label
belongs to CCSD(T). CCSD is now described as usually more accurate
than MP2 but without the perturbative triples.
- def2 ECP wording ("cover the whole periodic table") in help_content and
inorganic_guards: in PySCF def2-SVP/TZVP cover H-Rn except the
lanthanides Ce-Lu, with ECPs from Rb on (checked with
pyscf.gto.basis.load / load_ecp); no actinides.
- Density-fitting help: 0.008 kcal/mol is ~125x below 1 kcal/mol, not
"hundreds of times". The setup label no longer states that single
aspirin measurement as a general accuracy cost; it points to the
Help topic instead.
- Click-to-measure help (and the reset_picks docstring) said leaving
the Analysis tab clears picks; they are cleared when the Analysis
viewer is redrawn (new result, backend switch).
- TD-DFT result card labelled every state S1..Sn, including states from
an unrestricted / restricted-open-shell reference, which are not
singlets. New app_formatters.tddft_state_prefix() keeps "S" only for a
closed-shell restricted reference (older results without scf_variant
fall back to the saved <S^2>).
History TS/IRC chips (also listed here) were fixed with APP-15. The
UV-Vis tables in app_visualization.py still say S{i} (viz area; can use
tddft_state_prefix). Dihedral range (0-360 vs signed) left unchanged: it
is shared with PES Scan ranges and needs a convention decision.
Test: tests/test_app_formatters.py
test_format_tddft_states_are_not_called_singlets_for_open_shell (fails
on the old code). Text-only changes have no test.
Contributions:
- Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
- MOL/PDB export: a failed rdDetermineBonds.DetermineBonds (radicals such as NO, metals, some ions) was swallowed and the file reported "Saved". molecule_to_rdkit now falls back to DetermineConnectivity on a fresh molecule and records a note; the status line says "bond orders could not be determined; bonds written as single" (or "atoms only" if even connectivity fails). - Freeze atoms: "x" or "1-" surfaced int()'s raw "invalid literal for int() with base 10" text; parse_atom_list now names the bad token. - Local strings put into HTML unescaped: conda env name, GPU name and probe reason (can carry an exception text with <...>), calibration label in the status panel, and CheckList placeholder labels. The others listed (XYZ upload filename via _set_molecule, downloads.py) are already escaped on main. Already fixed on main: PNG metadata / reorg XYZ level from the live dropdowns (744e583). Not changed: theme.py global !important rules in notebook mode (suspected; needs a JupyterLab browser check). Tests: tests/test_appui_audit_low.py test_mol_export_warns_when_bond_orders_cannot_be_determined, test_mol_export_of_a_closed_shell_molecule_has_no_warning, test_freeze_list_typo_gets_a_readable_message, test_local_strings_are_escaped_in_html, and tests/test_structure_edit.py::...::test_parse_atom_list (extended); all new ones fail on the old code. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
- measurement.describe_picks caught ZeroDivisionError only for the dihedral; three picks with two coincident atoms raised ASE's "Undefined angle" (the click handler then showed a generic failure). The angle is now reported as "angle undefined - two atoms coincide". - app_pes_pick.push_scan_highlight and app_structure_edit's _push_edit_highlight sent their highlight by temporarily swapping app._measure_js_bridge. push_highlight now takes a bridge= argument and both callers pass their own Output; the Measure bridge is never reassigned. Not changed: c_stderr.py process-global fd 2 (documented trade-off); nmr_plot.py (VIZ-14, viz area). Tests: tests/test_measurement.py test_three_picks_with_coincident_atoms_report_undefined_angle and tests/test_appui_audit_low.py test_scan_and_edit_highlights_do_not_swap_the_measure_bridge (both fail on the old code). Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…w-up) The TD-DFT result card stopped calling open-shell excited states S1…Sn (UI-13), but the UV-Vis and ECD state tables in the Analysis panels still did. Both tables take a state_prefix; the panels set it from the live result or the saved result.json with app_formatters.tddft_state_prefix. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…udit DATA-10) run_calibration started each step's worker with daemon=True. A daemonic process may not have children, so run_freq_calc's parallel IR-intensity ProcessPoolExecutor always failed and fell back to serial: the tier-4 "parallel-IR" calibration entry only ever measured the serial path. The worker is now non-daemonic. To keep Stop/Skip/timeout from orphaning the pool's processes (verified: a plain terminate() of the worker leaves spawn pool children alive indefinitely), the spawn target _calibration_worker_main makes the worker lead its own process group and _terminate_worker_tree signals the whole group (POSIX; plain terminate() elsewhere). An atexit hook reaps a still-running worker at interpreter exit, which multiprocessing would otherwise join. Tests (tests/test_audit_low_data.py): run_calibration creates the worker with daemon=False (fails on the old code); _terminate_worker_tree kills a grandchild in the worker's process group. Manually confirmed a spawned H2O/STO-3G frequency worker now logs "Numerical IR intensities (parallel x2)". Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
save_result created the result directory, then called json.dumps on the payload. Any raw passthrough holding a numpy bool/array (cc_converged, solvent, spectra, extras) raised TypeError after mkdir, leaving an empty directory and losing the save. NaN/inf were written as bare NaN/Infinity (invalid strict JSON), a 300-char custom basis pushed the directory name past the filename limit (OSError), the write was not atomic, and save_trajectory raised on float32 coordinates/energies. Now the payload goes through _json_safe (numpy scalars/arrays -> plain Python, Path -> str, non-finite floats -> null; json.dumps(default=str, allow_nan=False)) BEFORE the directory is created; result.json is written via a temp file + os.replace and the directory is removed if that write fails; each formula/method/basis name component is capped at 48 chars. save_trajectory uses the same serialiser. Readers already treat a missing energy as None (formatters use _finite_or_none, comparison prints "-"). Tests (tests/test_audit_low_data.py): numpy fields, no empty dir on failure, NaN -> null, long basis name, float32 trajectory. All five fail on the old code. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…udit DATA-12) UserSettings.load caught only OSError/JSONDecodeError, so a settings.json with non-UTF-8 bytes (UnicodeDecodeError) or pathological nesting (RecursionError) escaped and crashed QuantUIApp construction. And save() rewrote the file from the three known sections, erasing any section or key a newer QuantUI (or a hand edit) had added. load now catches ValueError (covers both decode errors) and RecursionError and falls back to defaults. save merges the on-disk object first when it has the same schema version: unknown top-level sections and unknown keys inside known sections are kept, known keys take the instance's values. A different/missing schema version is still overwritten as before. Tests (tests/test_audit_low_data.py): non-UTF-8 and deeply nested files load as defaults; a save keeps future_section and compute.future_key. All fail on the old code. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…it DATA-13) - _append/prune wrote with plain json.dumps, so a numpy int/bool or a Path in a perf/event/prediction record raised TypeError out of the logger. Records now serialise with a default that maps numpy values via .tolist() and anything else to str. - _read_all and prune_events only skipped undecodable lines. A valid but non-object line (e.g. "123" in event_log.jsonl, "42" in perf_log.jsonl) made prune raise TypeError on every 20th event and broke estimate_time. Both now skip non-dict JSON values; prune also treats a non-string timestamp as malformed (kept) instead of raising. - _LOCK is per-process, so a prune in one kernel could drop an append made by another kernel/worker between its read and rewrite. Prune now keeps the original lines verbatim, re-reads any bytes appended after its read, and swaps the file in via temp file + os.replace. - _read_all returned a shallow copy of the cached list, so a caller mutating a record dict poisoned the cache; it now copies each record. Tests (tests/test_audit_low_data.py): numpy/Path event fields, non-object lines in event and perf logs, a foreign append during prune survives, and cache isolation. All four fail on the old code. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…ATA-10) test_poll_loop_checks_stop_event asserted the literal ".terminate()" in run_calibration. Since the DATA-10 fix the poll loop terminates through _terminate_worker_tree (which signals the worker's process group so its parallel IR pool dies too, falling back to worker.terminate()). The test now checks run_calibration calls the helper and the helper terminates. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…, no sub-second power law (audit DATA-14) - density_fit was partitioned in estimate_time_from_records but not passed to the frequency cost model's single-point anchor, nor by the estimator_eval replay, so DF and non-DF histories were mixed there. Both now pass it through. - The app and calibration logged n_cores=1 unconditionally and never recorded the thread count. They now record n_threads (PySCF's actual BLAS/OpenMP thread count, new calc_log.current_thread_count) and leave n_cores unset. n_threads is provenance only; an absent n_cores reads back as 1 in the estimator, so estimates are unchanged. (Normalising by the real thread count would change predictions and mix with existing n_cores=1 records -- left as a decision, not done here.) - Calibration geometry-opt steps put the optimizer step count in n_iterations; it is now logged as n_steps (n_iterations None), matching app records, so calibration runs feed estimate_opt_steps. - The n_basis^beta power-law strategies (1 and 3) took the median of elapsed*cores/nb^beta over all sizes; sub-second runs are dominated by fixed overhead, so extrapolating them grossly over-predicts large systems (four <0.1 s STO-3G runs predicted 51 min at nb=200). Records under 1 s (_MIN_POWER_LAW_ELAPSED_S) are now left out of the power-law fit; if fewer than two remain, the gentler electron-count strategies answer instead. Existing estimator tests pass unchanged. Tests (tests/test_audit_low_data.py): freq model DF partition, replay passes density_fit, sub-second power-law case, n_threads recorded, calibration opt steps/threads (parent and worker). All six fail on the old code. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…(audit DATA-15) - prune() only saw checkpoints load_all() could read, so a directory with a missing/corrupt meta.json (or an old schema) was never deleted. After the age/count passes, prune now removes such directories whose newest file mtime is past the age cutoff. Only names shaped like a resume key (16 hex chars) are touched, so a checkpoint root pointed at a shared directory never loses anything else; a just-created dir is fresh and kept. - _coords_digest rounded to 6 dp but kept the sign of zero, so -0.0 vs 0.0 (or a coordinate rounding to -0.0) gave a different resume key for the same geometry. Values are now normalised with "+ 0.0". (A checkpoint written before this with a -0.0 coordinate will not be offered for resume; it ages out normally.) - Not done: the suspected "running checkpoint resumable with no liveness check" item needs a pid/host/boot liveness design, not a small fix. Tests (tests/test_audit_low_data.py): sign-of-zero digest equality; prune removes old missing-meta and corrupt-meta dirs, keeps a fresh one and a non-key-named one. Both fail on the old code. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
test_a_collision_never_reuses_an_existing_directory searched only the first 4000 characters of save_result for the collision loop and mkdir. Since the DATA-11 fix serialises the payload before creating the directory, those lines sit further down; the test now inspects the whole function source. The assertions themselves are unchanged. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…er-less upload (audit DATA-16) Paste box (molecule.parse_xyz_input) and upload (structure_upload) rejected or mis-read common real-world files: - UTF-8 BOM (Windows Notepad) broke the XYZ count line; UTF-16 raised a raw decode error; old-Mac CR line endings, Fortran "1.17D-01" exponents and the Unicode minus (U+2212) were rejected. New molecule.normalize_xyz_text strips the BOM and normalises newlines and the minus; coordinates parse with a Fortran-D-aware float. Uploads are decoded once (UTF-16 by BOM, else UTF-8 with replacement) and written back as UTF-8 before the format readers see them. - D/T hydrogen isotope labels were invalid symbols. They are now read as H, with a note (paste message and upload notes) that standard isotope masses are used, so frequencies are those of the ordinary-H molecule. - Upload rejected the header-less "SYMBOL X Y Z" XYZ the paste box accepts; it now goes through parse_xyz_input. - PDB alternate locations were all loaded (H2O -> H2O2). Only altLoc 'A' (else the first seen) is kept, with a note. - An over-long upload filename raised a raw OSError outside the reader's try block. The stem is capped at 100 chars and the temp write moved inside the try. - A multi-frame XYZ took charge/multiplicity from frame 1's comment but the geometry from the last frame; the last frame's comment is used now. Not done: ORCA .out files are still read as Gaussian output (the audit lists it, but it needs an ORCA reader, not a parser tweak). Tests (tests/test_audit_low_data.py): paste quirks (BOM, CR, Fortran D, Unicode minus, isotopes), upload quirks (BOM, UTF-16, CR, Fortran D, Unicode minus, header-less), isotope note, last-frame charge comment, PDB alt-locs, over-long filename. 15 of 16 fail on the old code (CR upload already worked via ASE). Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
- quantui.student_friendly_resolve now exports the chain-backed resolver
the UI uses (structure_providers: bundled library, then PubChem/CACTUS),
not pubchem.py's PubChem-only function of the same name;
run_pes_scan / PESScanResult are added to __all__.
- Molecule.get_spin returned (mult-1)//2, so a doublet gave S=0. It now
returns S = (mult-1)/2 as a float (no callers). test_molecule's doublet
assertion encoded the old value and is updated to 0.5.
- structure_edit.parse_atom_list expanded a range before bounds-checking
("1-30000000" took seconds; larger ranges exhausted memory). Ranges are
now checked against the atom count (or a 100000 ceiling when unknown)
first.
- pubchem.smiles_to_xyz / inchi_to_xyz skipped embedding when
optimize_3d=False, so fetch_structure(conformer_3d=False) always raised;
embedding now always runs and only the UFF clean-up is optional. Local
embedding is capped at 500 atoms (with H) so a protein-sized SMILES
cannot tie up the kernel.
- issue_tracker used sqlite3.connect as a context manager, which commits
but never closes; a _connect helper now always closes.
- save_thumbnail called matplotlib.use("Agg"), switching the backend for
the whole kernel. It now renders a bare Figure on its own Agg canvas
without touching pyplot; test_savefig_failure_does_not_raise asserted
a pyplot close() and now asserts no pyplot figure is left open.
- The pyscf.log warnings digest matched "error"/"failed"/"imaginary" as
substrings; they are whole words now ("warn" stays a prefix).
- molecule_library.search passed "%"/"_" through as LIKE wildcards; they
are escaped and match literally.
- The calibration worker's log header used deprecated datetime.utcnow().
Already fixed: calibration.json location is overridable via
QUANTUI_CALIBRATION_PATH (742cabb, audit INFRA-1).
Not done (decision / browser needed): hostname in every pyscf.log header,
building the library store into the package dir on demand, unbounded
live_log text/DOM growth.
Tests (tests/test_audit_low_data.py): exports, doublet spin, range bounds,
SMILES without UFF + atom cap, closed sqlite connections, backend kept,
whole-word digest, literal wildcards. All fail on the old code (the old
parse_atom_list ran out of memory on the large range).
Contributions:
- Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
… mypy) all_job_ids() de-duplicated with `seen.add(j)` inside a list comprehension; mypy 1.10 (CI's pin) rejects using set.add's None value. Same behaviour with dict.fromkeys (unique, first-seen order). Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
…idge= Two interactions between this branch's batches: - The UI-14 MOL/PDB bond fallback added excepts in app_exports.py that report through a note, but they were unannotated, so the new per-file broad-except cap (INFRA-15) counted them. Marked as optional probes. - LEAD-4 replaced the measure-bridge swap in push_scan_highlight with a bridge= argument; the new app_pes_pick test (INFRA-22) still checked the swap. It now checks the bridge is passed and the measure bridge is never touched. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
The Windows job (no PySCF) failed five tests from this branch: - HPC-14 made the batch worker run the required single point after a geometry opt / pre-opt; test_audit_defects_issue19 (NMR pre-opt) and test_slurm_ingest_cl26 (geometry_opt round trip) now stub run_in_session like test_backends_worker already does. - test_pes_scan's progress-stream test calls run_pes_scan, which needs PySCF: skip without it, like its neighbours. - test_appui_audit_low keyed fake results by str(Path), which is \\a on Windows; key by as_posix(). - test_hpc_audit_low wrote its 'new attempt' log with write_text, which gives CRLF on Windows; write bytes. Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
NCCU-Schultz-Lab
added a commit
that referenced
this pull request
Nov 12, 2026
test_terminate_worker_tree_kills_grandchildren (from #172) failed on CI's py3.9 runner with ProcessLookupError: the child was reaped between os.path.exists("/proc/<pid>") and reading /proc/<pid>/stat, and reading the stat file of a just-reaped process raises ESRCH. A _running() helper reads the stat file once and treats any OSError as "gone". Contributions: - Claude (Opus 5.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 Opus 5.5 <noreply@anthropic.com>
This was referenced Nov 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the Low-severity findings from the 2026-11-10 codebase audit: 63 commits, roughly one per finding (bundled findings grouped), with regression tests for behaviour changes. Local
pytest -m "not network": 4931 passed, 11 skipped (py3.11). ruff, black, and mypy 1.10 (CI pin) are clean.Science / numerics
run_calc, PyFock ECP guard, GPU opt-out parsing, hygiene.Data / persistence
save_result.Batch / HPC
live.log,elapsed_s/warnings).quantui-batchcancel/rerun.App / UI / viewer
Tests / CI / packaging / docs
_safe_cb; version, pre-commit and 3Dmol checks..plansignored.app_pes_pickandbackends/local.Not done here (repo settings, larger refactors, or decisions)
_do_run.Needs a local / HPC check
Authorship
Generated by Claude Code