Skip to content

AUDIT F11 follow-up: support solvent in geometry_opt with required final single point - #122

Merged
jonathanschultzNU merged 6 commits into
mainfrom
claude/code-review-t1ji0e
Sep 9, 2026
Merged

AUDIT F11 follow-up: support solvent in geometry_opt with required final single point#122
jonathanschultzNU merged 6 commits into
mainfrom
claude/code-review-t1ji0e

Conversation

@jonathanschultzNU

@jonathanschultzNU jonathanschultzNU commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

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 against main.

Key Changes

Worker backend (quantui/backends/worker.py):

  • Added "geometry_opt" to _SOLVENT_SUPPORTED_CALC_TYPES (alongside "single_point" and "reorganization_energy")
  • Extended _run_geometry_opt() to run a required solvated single-point calculation on the optimized geometry when request.solvent is set
  • The solvated single-point energy and convergence status replace the optimizer's final values, ensuring the reported result reflects the solvated state
  • Raises RuntimeError if the required single point fails to converge

Test coverage (tests/test_backends_worker.py):

  • Updated test_solvent_on_unsupported_calc_type_returns_error to exclude "geometry_opt" from the rejection list (now supported)
  • Added test_solvent_on_geometry_opt_runs_required_final_single_point — verifies the solvated single point runs and its energy replaces the optimizer's result
  • Added test_solvent_on_geometry_opt_final_single_point_must_converge — ensures unconverged solvated single points fail the job

Result serialization (quantui/results_storage.py, quantui/backends/worker_payload.py):

  • Persisted cc_converged, td_converged, and n_converged_states to result.json (AUDIT F07/F08 follow-up)
  • These fields were previously only in-memory; reloaded History cards had no way to distinguish "no post-HF/TD ran" from "ran but didn't converge"

Result formatting (quantui/app_formatters.py):

  • Updated 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/DFT
  • Prevents misleading "SCF converged: No" labels when the SCF succeeded but post-HF/TD correlation failed
  • Added test coverage for all three label scenarios

TD-DFT convergence logic (quantui/tddft_calc.py):

  • Relaxed converged from "every root converged" to "at least one root converged" (AUDIT F08 code review)
  • Rationale: Davidson solvers routinely leave higher/harder roots short of the iteration budget while lower, physically relevant ones converge — a normal, fully valid result was being flagged "treat with caution"
  • Handles missing td.converged attribute gracefully (treated as "no information", not "unconverged")
  • Added test coverage for partial convergence and missing attribute scenarios

Performance optimization (quantui/orbital_visualization.py):

  • Cached ECP core-electron lookups per unique element instead of per atom
  • Hoisted pyscf.gto import out of the atom loop
  • Reduces redundant work for large molecules in cube export

Testing

  • pytest -m "not network" green (full suite, exit code 0)
  • ruff + black clean

Notes

  • Not app-verified (Voilà/GPU/NCShare) — cloud-only session

NCCU-Schultz-Lab and others added 6 commits September 7, 2026 22:56
…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
@jonathanschultzNU
jonathanschultzNU merged commit c770bfd into main Sep 9, 2026
5 checks passed
@jonathanschultzNU
jonathanschultzNU deleted the claude/code-review-t1ji0e branch September 9, 2026 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants