AUDIT F11 follow-up: support solvent in geometry_opt with required final single point - #122
Merged
Merged
Conversation
…additional concerns) Worker's _SOLVENT_SUPPORTED_CALC_TYPES omitted "geometry_opt" while app_runflow.py's UI enables (and relabels) the solvent checkbox for Geometry Opt, documenting a real "gas-phase optimization, solvated final single point" approximation. A user who checked the box and submitted to SLURM got a hard UNSUPPORTED_CAPABILITY rejection of the whole job, even though the identical settings succeed interactively. _run_geometry_opt now mirrors app.py's interactive _run_required_final_single_point: after the gas-phase optimize_geometry call, a required solvated single point runs on the final geometry via session_calc.run_in_session, its energy replaces the optimizer's last-step energy, and its convergence is folded into the overall result — an unconverged required single point fails the job rather than silently reporting the gas-phase result. "geometry_opt" is now in _SOLVENT_SUPPORTED_CALC_TYPES alongside "single_point" and "reorganization_energy". Updated the existing parametrized rejection test to drop "geometry_opt" (it's supported now) and added tests covering the new success path and the "final single point must converge" failure path. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E7tooXTTNTfemGJstUib8F
…h missing per-root data (audit additional concerns) AUDIT F08 made overall `converged` require ALL requested TD roots to converge, but a Davidson solve for more than a few states routinely leaves the higher/harder roots short of the default iteration budget while the lower, physically relevant ones are fine — so a completely normal multi-state UV-Vis run was always flagged "treat with caution". Separately, `td_converged is not None and all(td_converged)` forces converged=False whenever the installed PySCF doesn't expose `td.converged` at all, since `td_converged is not None` is then always False — flagging every TD-DFT/TDHF result on such a build regardless of how well it actually converged. Relaxed the rule to: SCF converged AND the TD solve produced states AND (no per-root info available, OR at least one requested root converged). This still catches the original F08 bug (every root came back unconverged), no longer penalizes a build with no td.converged attribute (treated as "unknown", not "unconverged"), and n_converged_states still reports the exact per-root tally for anyone who wants it. Added tests for partial-convergence (deterministically reproduced by patching TDHF.kernel to overwrite td.converged after a real, normal solve, since Davidson's actual partial-convergence behavior isn't reliably reproducible across PySCF versions) and for a PySCF build without a td.converged attribute. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E7tooXTTNTfemGJstUib8F
…n saved-result cards (audit additional concerns) format_past_result still hardcoded a "SCF converged" row label, but data["converged"] for saved post-HF/frequency/TDDFT results already folds in CC-amplitude (F07), TD-root (F08), or Hessian (F15) convergence — exactly the mislabel those fixes corrected on the live cards (format_result/format_freq_result/format_tddft_result now say "Converged"). A saved CCSD result whose SCF converged but whose CC amplitudes did not rendered "SCF converged: No", wrongly implying the SCF itself failed; the same applied to frequency (Hessian-only) and TDDFT (TD-only) History cards. format_past_result now mirrors each live formatter's label choice: "Converged" for frequency and tddft (unconditional, since both always fold extra status in), "Converged" for a single-point card that carries cc_converged, and "SCF converged" otherwise. That label logic needs cc_converged/td_converged/n_converged_states to actually be present on saved result.json payloads — results_storage. save_result never persisted them (they existed only on the in-memory result object), so added them alongside the existing mp2/ccsd correlation fields. Also plugged the matching gap one hop upstream on the SLURM path: worker_payload.tddft_result_payload never serialized td_converged/n_converged_states into the staging JSON at all, and slurm_ingest._basic_result didn't forward them into the reconstructed result — both now round-trip through to History like cc_converged already did. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E7tooXTTNTfemGJstUib8F
…ge_and_spin (audit additional concerns) infer_charge_and_spin imported pyscf.gto and called load_ecp inside the per-atom loop, so an N-atom molecule with a basis argument repeated both the import and the ECP-table lookup once per atom instead of once per unique element — needless repeated work on the interactive cube-export "Generate" path for a large molecule. The pyscf.gto import now runs at most once (only when a basis is given), and each unique element's core-electron count is computed once and cached, with every atom of that element served from the cache. Added a test that counts load_ecp calls per element symbol. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E7tooXTTNTfemGJstUib8F
quantui/orbital_visualization.py:609 — `_ecp_core_electrons: dict = {}`
left the dict untyped, so mypy inferred `dict[Any, Any]` and flagged
`_ecp_core_electrons[sym]` as returning `Any` from a function declared
`-> int` (no-any-return), failing CI's Lint & type check job
(mypy~=1.10.0, python_version=3.9 per pyproject.toml). Typed the cache
as `dict[str, int]`.
Verified against the pinned mypy==1.10.1 with the repo's exact
warn_return_any/python_version=3.9 config on an isolated repro of the
function — no issues.
Contributions:
- Claude (Sonnet 5): code edits, review, and conceptual discussion
- Jonathan Schultz: overall vision, planning, review, and orchestration
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01E7tooXTTNTfemGJstUib8F
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
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
Implements solvent support for geometry optimization calculations by mirroring the interactive app's approximation: optimize in gas phase, then run a required solvated single point on the final geometry. This resolves an inconsistency where the UI enabled the solvent checkbox for geometry_opt but the worker would reject or silently ignore it.
Replaces #121, which targeted
claude/gpt-astra-audit-fixes-iqa1om(now merged via #120 and deleted) and can no longer be reopened/retargeted since GitHub won't change the base of a closed PR once its base branch is gone. Same branch (claude/code-review-t1ji0e), now againstmain.Key Changes
Worker backend (
quantui/backends/worker.py):"geometry_opt"to_SOLVENT_SUPPORTED_CALC_TYPES(alongside"single_point"and"reorganization_energy")_run_geometry_opt()to run a required solvated single-point calculation on the optimized geometry whenrequest.solventis setRuntimeErrorif the required single point fails to convergeTest coverage (
tests/test_backends_worker.py):test_solvent_on_unsupported_calc_type_returns_errorto exclude"geometry_opt"from the rejection list (now supported)test_solvent_on_geometry_opt_runs_required_final_single_point— verifies the solvated single point runs and its energy replaces the optimizer's resulttest_solvent_on_geometry_opt_final_single_point_must_converge— ensures unconverged solvated single points fail the jobResult serialization (
quantui/results_storage.py,quantui/backends/worker_payload.py):cc_converged,td_converged, andn_converged_statesto result.json (AUDIT F07/F08 follow-up)Result formatting (
quantui/app_formatters.py):format_past_result()to use context-aware convergence labels: "Converged" for frequency/tddft/CC methods (where the label folds multiple convergence criteria), "SCF converged" only for plain HF/DFTTD-DFT convergence logic (
quantui/tddft_calc.py):convergedfrom "every root converged" to "at least one root converged" (AUDIT F08 code review)td.convergedattribute gracefully (treated as "no information", not "unconverged")Performance optimization (
quantui/orbital_visualization.py):pyscf.gtoimport out of the atom loopTesting
pytest -m "not network"green (full suite, exit code 0)Notes