M-ISSUES backlog + M-THEME token map - #59
Merged
Merged
Conversation
Three separate label->key dicts had drifted apart: app_runflow's _CALC_TYPE_CANON (header banner) and an inline dict in app.py's _do_run (prediction-log calc type) duplicated calc_type_key()'s canonical _CALC_TYPE_KEYS mapping, which already includes "Reorganization Energy" but the duplicates existed only as a maintenance hazard now that all three agree. Both call sites now delegate to _CALC_TYPE_KEYS / calc_type_key() instead of carrying their own copy. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
estimate_time_from_records() partitions its pool by source (and density_fit) for every calc type, but the frequency-cost-model fallback called _estimate_frequency_cost() without threading source through — the function already accepts and uses it. A Frequency estimate for an app run was drawing its single-point anchor from the mixed app+ calibration pool (which includes fresh-subprocess import cost), systematically inflating small-molecule frequency estimates — exactly the bias the source field exists to remove. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…SUE.3) session_calc.SessionResult recorded whether density fitting was used; TDDFTResult, NMRResult, FreqResult, and OptimizationResult discarded try_density_fit()'s flag instead of storing it. A DF-on TD-DFT / freq / NMR / geometry-opt run was logged as unfitted: the perf record stored density_fit=None, the result card's "RI" badge never showed even though the run header printed "Density fitting: RI", and the estimator's DF partition could never match those calc types. Also: results_storage.save_result() never persisted density_fit into result.json at all, for any calc type (including single-point) — so even a live SessionResult's RI badge would vanish on History replay. Added it alongside the existing gpu_used field. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
_prepare_scf_checkpoint's loaded density was passed straight into mf.kernel(dm0=_dm0) inside the same try/except that turns any SCF failure into a hard RuntimeError. A warm-start-specific rejection (a GPU-migrated mean-field rejecting a host/numpy density, or a loadable but incompatible chkfile density) aborted an otherwise-fine calculation — contradicting the module's own "a warm start is an optimisation, not a requirement." Now a warm-start failure logs a warning, drops dm0, and retries mf.kernel() from a scratch guess before giving up. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
A freshly constructed BFGS instance always starts nsteps at 0, even on a resumed run. Left unseeded, the per-step checkpoint.update(steps_done =dyn.nsteps) callback reported a count that restarted at 0 instead of continuing from what the interrupted run had already banked — a resumed-then-interrupted-again optimization under-reported how much work was saved. Seeding dyn.nsteps from the checkpoint on resume fixes it. The originally filed duplicate-boundary-frame half of this issue does not reproduce against the pinned ASE version: Dynamics.irun() already guards its pre-loop observer call with _traj_is_empty(), so appending to an already-populated trajectory on resume correctly skips that write. Verified directly against real PySCF/ASE (steps=1, compressed H2/STO-3G, run -> resume): trajectory length after resume was already old+1 without this change, but steps_done regressed 1 -> 1 instead of 1 -> 2. New regression test covers the step-count behaviour and keeps the trajectory-length invariant as a live check. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Every other one-off action button (cancel, basis-fix, spin-suggest, spin-apply, ...) wraps its .on_click callback in _safe_cb so an unexpected exception is logged instead of escaping into ipywidgets' click dispatch. The resume-list restore/discard buttons skipped it. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
past = history; history = history + [record] allocated a fresh length-i list on every one of n iterations (~n^2/2 element copies), making `python -m quantui.estimator_eval` needlessly slow on a large perf_log.jsonl. Appends the current record in place after it has already been used as the prediction's history snapshot instead — same behaviour (a prediction never sees its own ground-truth record), linear time. estimate_time_from_records() only reads its records argument, so sharing the growing list by reference across iterations is safe. Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…UE.8) connectivity._hill_formula, app_runflow._formula_from_symbols, and Molecule.get_formula each implemented the same C,H-first Hill order with inline subscripts. Renamed connectivity._hill_formula to the public hill_formula() and made the other two delegate to it, so a convention change (e.g. a different subscript style) only has to be made once. _formula_from_symbols keeps its "?" fallback for an empty symbol list; hill_formula([]) itself still returns "" (matching Molecule.get_formula's prior empty-molecule behaviour). Contributions: - Claude (Sonnet 5): code edits, review, and conceptual discussion - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…roadmap 14)
Execution Sequence step 1 ("define one theme token map... route hardcoded
hex values through the tokens"). Adds 22 new tokens to theme.py alongside
the existing BORDER/BORDER_STRONG: a text-tier greyscale set (TEXT_HEADING
through TEXT_SLATE_DARK), BG_PANEL, BORDER_LEGACY, and a status-accent set
(ACCENT_ERROR/_ALT, ACCENT_SUCCESS/_BG/_ALT, ACCENT_WARNING/_LIGHT,
ACCENT_INFO, ACCENT_PURPLE, ACCENT_TEAL).
Migrates the 9 widget-building "chrome" modules (app.py, app_builders.py,
app_formatters.py, app_runflow.py, descriptor_cards.py, help_content.py,
app_analysis.py, app_history.py, calc_log.py) to reference these tokens
instead of scattered literal hex — 379 of ~436 occurrences in that file
set (the 22 highest-frequency distinct values; a long tail of ~70
low-frequency values is left for a follow-up pass). _APP_CSS's three new
substitutions use the pre-existing sentinel + .replace() mechanism, same
as the existing __Q_BORDER__ sentinel, since that block is too CSS-brace-
dense for an f-string.
Each token keeps its call sites' exact original value -- this is a pure
extract-to-constant refactor with zero rendered-pixel change, not a
harmonization pass, so it's safe to do without a browser (verified by
construction: ast.parse on every touched file, a case-insensitive re-scan
confirming zero raw occurrences of the 22 target values remain outside
theme.py, full suite green). A later, visually verified pass can still
collapse near-duplicate tiers into fewer WCAG-measured values the way
BORDER/BORDER_STRONG already were.
Deliberately excludes the plotting/3-D-viewer files (analytics.py,
orbital_visualization.py, app_visualization.py, visualization_py3dmol.py,
ir_plot.py) -- a wrong substitution there risks an actual rendering
regression this session has no browser to catch.
New regression tests in test_theme_contrast.py: widened sentinel-leak
guard, and a standing scan (TestNoRawHexReintroducedInMigratedChrome)
that fails if any of the 22 migrated values reappear as a raw literal in
the 9 migrated files.
Contributions:
- Claude (Sonnet 5): code edits, review, and conceptual discussion
- Jonathan Schultz: overall vision, planning, review, and orchestration
Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
NCCU-Schultz-Lab
changed the title
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
1 participant
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
Two unrelated pieces of work landed on this branch in the same session — kept as one PR rather than opening a second for the same branch.
M-ISSUES: clear the 8-item standing backlog
Closes out all 8 items in the M-ISSUES standing backlog (defects and maintainability findings surfaced by a code review of the M-METAL branch against pre-existing M-CHECKPOINT/M-DF/M-PROGRESS code — none are M-METAL regressions). One commit per issue.
source=through to_estimate_frequency_cost, removing a bias that inflated small-molecule frequency estimates.density_fitis now recorded on TD-DFT/NMR/Frequency/Geometry-Opt results, not just single-point. Also fixes a separately-found gap:results_storage.save_result()never persisteddensity_fitintoresult.jsonfor any calc type, so the "⚡ RI" badge vanished on History replay even for single-point runs.steps_doneregression on geometry-opt resume (seedsdyn.nstepsfrom the checkpoint). The duplicate-boundary-frame half of the original filing does not reproduce against the pinned ASE version — verified directly with a real PySCF run→resume — since ASE's ownDynamics.irun()already guards that case. New regression test added either way._safe_cb, consistent with every other action button.estimator_eval.replay()is O(n) instead of O(n²).connectivity.hill_formula);Molecule.get_formulaandapp_runflow._formula_from_symbolsboth delegate to it.M-THEME: token map for the widget-building "chrome" files (roadmap 14, Execution Sequence step 1)
Adds 22 new tokens to
theme.py(a text-tier greyscale set,BG_PANEL,BORDER_LEGACY, and a status-accent set) and migrates 9 widget-building modules (app.py,app_builders.py,app_formatters.py,app_runflow.py,descriptor_cards.py,help_content.py,app_analysis.py,app_history.py,calc_log.py) to reference them instead of scattered literal hex — 379 of ~436 hardcoded-hex occurrences in that file set.Each token keeps its call sites' exact original value — a pure extract-to-constant refactor with zero rendered-pixel change, not a harmonization pass, so it's safe without a browser. Deliberately excludes the plotting/3-D-viewer files (
analytics.py,orbital_visualization.py,app_visualization.py,visualization_py3dmol.py,ir_plot.py) — a wrong substitution there risks an actual rendering regression this session has no browser to catch. New standing regression test (TestNoRawHexReintroducedInMigratedChrome) guards against the migration eroding.Test plan
pytest -m "not network"— 2587 passed / 24 skipped / 0 failed / 0 errors (2611 collected)main, zero newast.parseon every touched file, a case-insensitive re-scan confirming zero raw occurrences of the 22 target values remain outsidetheme.py