Orbital isosurface: image export, live controls, and py3Dmol-only rendering (v0.6.0) - #43
Merged
Merged
Conversation
Two changes that arrived together and touch the same files, so they ship together rather than as an artificial split. ── ORBX.1: client-side PNG export ────────────────────────────────────── A Save-PNG button inside the isosurface viewer, capturing the canvas exactly as rotated. It lives on the viewer rather than in the control row because it has to: pngURI() reads the live canvas, and a Python button would need kernel -> JS -> kernel, which the kernel cannot do synchronously. That needed a JS -> kernel direction this codebase did not have. Route C (M-LOGSCROLL) goes the other way. The mechanism here is the standard ipywidgets one: JS writes into a hidden Textarea's DOM node and dispatches an 'input' event, which the widget's own view already listens for, so it syncs like a user typing. The button is opt-in by construction — without a capture_class no button renders at all, so it cannot ship somewhere with nowhere to deliver. ── ORBX.2: tunable resolution ────────────────────────────────────────── Four grid presets (40/60/80/100 per axis), persisted via UserSettings, with cost multipliers in the labels because the jump is steep and the wait is what users are choosing between. The roadmap's two-knob warning is narrower than written. Only the Plotly path strides a cube down to a point cap; py3Dmol isosurfaces in-browser at full resolution, so there a finer grid is visible immediately. The cap is fixed anyway: it now scales with grid VOLUME (100^3 is 4.6x the points of 60^3, not 1.7x) and is ceilinged at 250k, since a fallback that locks the tab is worse than one that is smooth and slightly coarse. No settings schema bump. The loader reads every viz key with a default, so a v1 file written before iso_resolution existed still loads — verified against an actual old-format file rather than assumed. ── py3Dmol-only for orbitals and vib export ──────────────────────────── User: "plotlymol is really only good for the molecule viewing windows." ORBITAL_ISOSURFACE and VIB_EXPORT are now single-backend in _TASK_POLICY, so preference is ignored for both. Molecule viewing still honours it, and a test asserts the narrowing does not leak. The router change alone was not enough for vib export: build_vib_export_html chose its own backend independently, so it would have kept emitting Plotly animations while the router said otherwise. The two dead paths are treated differently on purpose. The Plotly isosurface renderer is kept and still tested — reverting is one line, and a test asserts the function still exists so it stays that cheap. The Plotly vib-export branch is removed, because it duplicated animation-building logic that would rot silently behind a flag. Still plotlymol-only and deliberately untouched: TRAJECTORY_EXPORT. The same principle applies to it, but it was outside the request. ── Tests ─────────────────────────────────────────────────────────────── Two caught themselves passing for the wrong reason. Mine wrote settings JSON without _schema_version, so the loader discarded the file and returned defaults — the "invalid value falls back" assertion passed without the validator ever running. The existing to_dict test pinned the exact viz dict, which is precisely what a new field should trip. Full suite: 2056 passed, 17 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two bugs from live testing. ── Backgrounds did not follow the theme ──────────────────────────────── py3Dmol paints the background INTO the WebGL scene at render time rather than reading it from CSS, so nothing re-reads it on a theme toggle. _rerender_plotly_theme already refreshed the orbital energy diagram, the IR/UV charts, the PES plot and the molecule viewer — but not the isosurface or the vibrational animation, which is why they stayed stale until the user happened to regenerate one. Both now re-render, and both stay cheap by construction: the isosurface re-reads the cube already on disk and never re-runs cubegen. That is the whole design constraint — cubegen is 15-30 s at the default grid and ~4.6x that at the finest, so regenerating on a theme click would make the toggle unusable. A test asserts the re-render path cannot reach the generate path. The vib viewer needed a full rebuild rather than __quantuiVibSetMode. That bridge switches frames on the existing viewer client-side — which is exactly why the camera survives a mode change — but it never touches the scene background, so it could not have fixed this. ── The page jumped on Generate Isosurface ────────────────────────────── on_iso_generate did clear_output() then display() to show the "Generating…" placeholder. That leaves the panel empty for a moment: it collapses from ~660px to zero, the document shrinks, the browser clamps scrollTop to the new maximum, and the returning content does not scroll back. The codebase had already solved this. _set_html_output swaps `outputs` in one assignment specifically "so the browser never observes an intermediate empty state", and its docstring cites the IR-toggle flicker this same pattern caused. The isosurface path simply never adopted it — four call sites converted. The one remaining bare clear_output() is a genuine reset when a new result loads, where empty is correct. A regression test greps for the clear-then-repopulate shape on that output so it cannot creep back. Full suite: 2062 passed, 17 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Six controls in the isosurface accordion: isovalue, opacity, transparent background, PNG filename, PNG dpi — plus a readout that explains what the isovalue actually means. ── Isovalue, expressed as enclosed density ───────────────────────────── The request was for "the amount of density contained within the isosurface". An isovalue is an amplitude threshold on psi; the density enclosed is the integral of |psi|^2 inside the surface. Those are different numbers, and the cube already on disk gives the second from the first for free — so the slider stays on isovalue (the quantity that goes in a methods section) and a label reads "encloses 97.4% of the density" beside it. Inverting the mapping to make the SLIDER a percentage is possible; this way both numbers are visible. ── The appearance controls never recompute ───────────────────────────── Isovalue, opacity and transparency are properties of the render, not of the grid, so all three redraw the cube on disk rather than re-running cubegen. That difference — sub-second versus 15-30 s, and ~4.6x that at the finest grid — is the entire reason they can be sliders instead of sitting behind an Apply button. A test asserts the redraw path cannot reach the generate path. All three render sites (first draw, slider redraw, theme redraw) now read from one iso_render_options(), so they cannot disagree about what the viewer should look like. ── Transparency is real, not simulated ───────────────────────────────── 3Dmol's WebGL context is created with alpha:true and preserveDrawingBuffer (checked in the vendored bundle), so setBackgroundColor(colour, 0.0) produces genuinely transparent pixels that survive pngURI(). Verified the alpha argument is emitted, and that re-encoding the PNG to stamp dpi preserves RGBA — flattening it there would have silently undone the whole option, so that has its own test. ── DPI sets print size, not pixel count ──────────────────────────────── Written into the PNG pHYs chunk via Pillow. Re-encoding cannot invent detail: at 300 dpi a 760 px capture declares itself 2.5 inches wide, which is what makes it land correctly in Word or LaTeX. Genuinely more pixels would mean supersampling the canvas before capture — a different change, and better refused than implied. A failed stamp returns the original bytes: losing metadata is mild, losing the export is not. ── Robustness found by tests ─────────────────────────────────────────── iso_render_options promised to fall back to defaults for missing widgets but only handled missing ATTRIBUTES — a widget holding an uncoercible value raised out and took the render with it. It now coerces to the default's type. Likewise the filename path assumed a str. One older test asserted the capture landed byte-for-byte; deliberate re-encoding made that the wrong invariant, so it now compares pixels. Also moved TRAJECTORY_EXPORT in the router test lists, missed when its policy changed. _PLOTLYMOL_ONLY_TASKS is now empty and kept deliberately, so restoring such a task stays a one-line change with live coverage. Full suite: 2088 passed, 23 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Closes out the round of live-testing feedback on the isosurface panel. ── Live controls, one JS bridge ──────────────────────────────────────── Isovalue, opacity, colours and background now update the EXISTING viewer rather than re-rendering it. A Python re-render replaces the viewer, so it cannot preserve the camera (GOTCHAS: camera state does not survive an atomic HTML swap) — the bridge saves getView()/setView() around a surface rebuild instead. No output swap also means no collapse and no scroll jump. That rewrite came out of a measurement: building the viewer from Python embedded the cube THREE times — once for addModel, once per addVolumetricData — which is ~39 MB of HTML per render at the 100^3 grid. Embedding once and driving 3Dmol from JS is what made the finer grids usable and the live controls possible at all. ── Bugs found by live testing ────────────────────────────────────────── Surfaces stacked instead of being replaced. addVolumetricData routes to addIsosurface, which does this.shapes.push(...) — an isosurface is a SHAPE, not a surface — so removeAllSurfaces() iterated an empty collection and removed nothing. One cause for every symptom reported: a lower isovalue engulfed the old surface (looked like it worked), a higher one hid inside it (looked dead), and stacked translucent layers read as steadily more opaque, so toggling the palette brightened the surface. Shapes are now tracked and removed by reference. The busy overlay could stick. The bridge retries up to 2 s for its hook; a busy(true) issued while no viewer existed kept polling and fired against the NEXT viewer, after busy(false) had run. A sequence guard makes only the newest call able to apply, and a fresh viewer bumps the sequence so nothing in flight can land on it. Transparency changed the live view. It is an export property now: the capture hook drops the background, grabs the canvas and restores it, all inside one synchronous call, with the restore in a finally. ── Panel behaviour ───────────────────────────────────────────────────── The molecule is shown before the first Generate, so the panel is never empty and the first isosurface fades in over an existing viewer. It also lets the user orient the structure first — the camera carries across, keyed on a scene hash so a different ORBITAL keeps the view and a different MOLECULE re-frames. Generate moved above the viewer, the viewer is framed like the others, and a Cancel button abandons an in-flight generation. cubegen cannot be interrupted, so Cancel bumps the render token: the result is discarded on arrival and the controls return immediately. ── A regression I caused, and why ────────────────────────────────────── Making Isosurface the default panel for geometry_opt took three tries. _PANEL_REGISTRY order is load-bearing twice: the FIRST auto_select that returns True wins, AND _pop_energies is what loads the orbital state _pop_isosurface checks. I first put Isosurface last (no effect), then first (which broke the dependency and reported "required data is missing" on results that had it). Correct order is Energies, Isosurface, Trajectory. Both of my tests had encoded my assumption rather than the rule documented directly above the code; both are fixed, and the dependency now has its own test. ── Version ───────────────────────────────────────────────────────────── 0.6.0, not a patch: plotlymol is no longer selectable for orbitals and both animation exports changed renderer. That is a deliberate reduction in user-facing choice, which a patch number would not communicate. Full suite: 2116 passed, 23 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Closes M-ORBEXPORT ORBX.1, ORBX.2 and ORBX.6, plus the py3Dmol-only routing decision and nine bugs found in live testing.
What a user gets
Two changes that reduce choice, deliberately
Orbital isosurfaces are py3Dmol only. Plotly downsamples the volume by construction and cannot carry the image export. Reverting is one line in
_TASK_POLICY; the Plotly renderer is kept and still tested, with a test asserting it still exists so the revert stays cheap.Vibrational and trajectory exports now come from py3Dmol — the renderer that drew them on screen. The Plotly export branch was removed rather than left unreachable, because it duplicated animation-building logic that would rot behind a flag.
These are why this is 0.6.0 and not a patch.
The measurement that shaped the implementation
Building the viewer from Python embedded the cube three times — once for
addModel, once peraddVolumetricData— which is ~39 MB of HTML per render at the 100³ grid. Embedding it once and driving 3Dmol from JS is what made the fine grids usable and made live controls possible, since the camera cannot survive a Python re-render (GOTCHAS: camera state does not survive an atomic HTML swap).Bugs, and what caused them
addVolumetricData→addIsosurface→this.shapes.push(...): an isosurface is a shape, not a surface, soremoveAllSurfaces()iterated an empty collection. One cause for every symptom — a lower isovalue engulfed the old surface, a higher one hid inside it, and stacked translucent layers read as ever more opaque.clear_output()+display()collapsed the panel;_set_html_outputalready existed for exactly this.Reviewer notes
A regression I caused, twice.
_PANEL_REGISTRYorder is load-bearing in two ways: the firstauto_selectthat returns True wins, and_pop_energiesloads the orbital state_pop_isosurfacechecks. I put Isosurface last (no effect), then first (broke the dependency — "required data is missing" on results that had it). Both of my tests had encoded my assumption rather than the rule documented directly above the code. Correct order is Energies → Isosurface → Trajectory, and the dependency now has its own test.Honest limits, stated rather than implied: DPI sets print size, not pixel count. Cancel discards the result but cannot interrupt PySCF.
standalone_htmlwas a no-op pass-through, so exports were bare fragments — now complete UTF-8 documents, which matters because they carry⇄and→.Still open: ORBX.7 (surface finish) — 3Dmol's Lambert shading has no specular term, so "metallic" is not achievable on this renderer. Scoped with the evidence in the roadmap.
Full suite: 2116 passed, 23 skipped.
🤖 Generated with Claude Code