Conversation
…#684) The dat.GUI opacity slider and the `o` shortcut both drive the `dataAlpha` uniform. The volume shaders apply it (`vColor *= dataAlpha`), but the `surface_vertex` fragment shader only declared it and never read it, so opacity was a silent no-op for Vertex, Vertex2D and VertexRGB data. - shaderlib.js: scale the per-vertex data colour by `dataAlpha` before compositing over the curvature. `vColor` is a varying, so scale into a local `dColor`. Scaling all four channels keeps the premultiplied-alpha convention of the volume path, so opacity 0 shows curvature only. - mriview_surface.js: `toggleOpacity` now remembers the last visible opacity instead of rounding it (0.7 -> 0 -> 0.7, not 0.7 -> 0 -> 1), and no longer leaks an implicit global. - test_webgl_headless.py: regression test rendering a saturated Vertex at opacity 1 -> 0 -> 1 and counting coloured pixels. Fails on main (53,760 red pixels at opacity 0) and passes with the fix. Closes #684 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P8oA4mUvTLd7sbs9iFaqM8
…ed, image-relative quantities Address review on #685: the 1000 / 50 / 1000 pixel-count thresholds were unexplained. The assertions now use named constants derived from the measured renders and related to the image size or to each other: - opacity 1: red-dominant pixels must cover >= 10% of the frame (measured ~27% at this view; curvature-only renders give 0%). - opacity 0: at most 1% of the opacity-1 count (measured 0). - opacity restored to 1: within 5% of the original count, which is a stronger check of the round trip than the previous "> 1000". Re-verified: fails against the pre-fix shader (53,760 red pixels remain at opacity 0), passes with the fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P8oA4mUvTLd7sbs9iFaqM8
…views Rule: a NaN anywhere at a voxel/vertex (data, either 2D dimension, any RGB channel, or the alpha map) renders fully transparent in both renderers; otherwise the alpha (2D colormap, alpha= kwarg, RGB alpha channel) is honored. - Volume2D/Vertex2D: alpha= is a real attribute (not an ndarray in attrs, which crashed the WebGL viewer with a 500); multiplied into the colormap alpha in quickflat for both classes, shipped to WebGL as a normalized float brain and applied in the shaders (folded into the nanmask attribute for vertex data: 2D vertex views already use 15 of the 16 guaranteed vertex attributes and adding more made the program fail to link silently). Saved/restored in HDF. - VolumeRGB/VertexRGB: NaN mask is applied without writing into the temporary returned by .volume for masked (linear) alpha Volumes; single- frame alpha is broadcast against multi-frame NaN masks (closes #629); NaN inside the alpha map -> 0 explicitly; color_voxels no longer mutates the caller's alpha array. - quickflat: RGBA is averaged across thickness in premultiplied space (no dark halos around transparent voxels, matching the WebGL compositor); make_svg no longer indexes arr[..., 3] on 2-D scalar images. - Dataview2D.to_json keeps vmin/vmax of exactly 0. - Package deduplicates byte-identical brains (Vertex2D(x, x) / VertexRGB(r, r, r) used to be reordered twice and crash the viewer). - WebGL hover/click readout shows NaN for masked vertices instead of 0. - Tests: browser-free NaN/alpha unit tests, quickflat-vs-WebGL parity tests, and dataset-switching tests checking that NaN masks and alpha never leak between datasets (setData and addData). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KdjyZMFXsn6mRMJu9i8ige
… by default The WebGL multi-layer sampling summed the layer samples, so one NaN voxel at any depth made the whole fragment transparent (quickflat's nanmean=False). surface_pixel now averages only the valid (non-NaN) layer samples and is transparent only when none is valid, matching quickflat's nanmean=True. A `nanmean` toggle in the surface controls (on by default) restores the old any-NaN-is-transparent behavior, and quickflat's make_figure / make_flatmap_image / add_data default to nanmean=True so both renderers agree. NaN-free data renders identically either way. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KdjyZMFXsn6mRMJu9i8ige
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KdjyZMFXsn6mRMJu9i8ige
There was a problem hiding this comment.
Pull request overview
Unifies NaN transparency and alpha handling across quickflat, WebGL, 2D, and RGB dataviews.
Changes:
- Adds normalized alpha maps and consistent NaN masking.
- Introduces valid-layer averaging with a
nanmeancontrol. - Adds HDF persistence, packaging fixes, documentation, and regression tests.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
examples/datasets/plot_data_with_alpha.py |
Demonstrates 2D alpha maps. |
docs/dataset.rst |
Documents NaN and alpha behavior. |
cortex/webgl/resources/js/shaderlib.js |
Adds alpha sampling and valid-layer averaging. |
cortex/webgl/resources/js/mriview.js |
Reports masked vertex values as NaN. |
cortex/webgl/resources/js/mriview_surface.js |
Adds the WebGL nanmean control. |
cortex/webgl/resources/js/dataset.js |
Loads alpha data and combines vertex masks. |
cortex/webgl/data.py |
Deduplicates packaged brains. |
cortex/tests/test_webgl_switching.py |
Tests dataset-switching isolation. |
cortex/tests/test_webgl_nan_alpha_parity.py |
Tests renderer parity. |
cortex/tests/test_webgl_data.py |
Tests package deduplication. |
cortex/tests/test_nan_alpha.py |
Covers NaN and alpha behavior. |
cortex/quickflat/view.py |
Defaults quickflat to nanmean=True. |
cortex/quickflat/utils.py |
Adds premultiplied RGBA averaging. |
cortex/quickflat/composite.py |
Propagates the new default. |
cortex/dataset/views.py |
Restores 2D alpha from HDF. |
cortex/dataset/viewRGB.py |
Fixes RGB masking and broadcasting. |
cortex/dataset/view2D.py |
Adds first-class 2D alpha support. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… package dedup - quickflat: nanmean now applies to dataviews that reach the RGBA branch. 2D/raw conversions carry their NaN mask, so nanmean=True averages the valid voxels only and nanmean=False hides any pixel touched by a NaN voxel; native RGB (NaN already alpha 0) treats fully transparent voxels as missing, like the WebGL RGB textures. - WebGL surface_pixel: NANMEAN also applies to RGB textures (fully transparent layer samples are skipped; validity from the current frame since an unbound next-frame sampler reads as opaque black). - WebGL vertex data: the nanmask combines the masks of both frames being blended (next frame only while framemix > 0), so a NaN in the next frame no longer interpolates towards a fake 0. - _mask_alpha: uint8 alpha maps are filled with byte 0 (their inferred vmin is a percentile of the bytes, 255 for a constant map). - Package: same-name brains are deduplicated only when their class, subject, transform, shape, dtype and mask match; otherwise raise a clear ValueError instead of serving one brain with another's metadata. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KdjyZMFXsn6mRMJu9i8ige
|
Addressed the Copilot review in the latest commit:
Each has a test ( |
|
@claude review |
|
Claude finished @mvdoc's task in 5m 3s —— View job Review
Note: I couldn't execute Copilot's 5 findings — all look correctly fixed in 814e53b
Each fix has good comments explaining why (not just what), and each is covered by a new/extended test. Rest of the diff
Minor, non-blocking observations
Nothing here looks like it needs another round — the fixup commit addresses all 5 Copilot findings substantively rather than superficially, and the surrounding code is well-tested and documented. |
Conflicts were limited to three quickflat signatures, where main's typing pass (#682) and this branch's `nanmean` default change touched the same lines. Resolved by keeping main's annotations with `nanmean=True`: - `quickflat.composite.add_data` - `quickflat.utils.make_flatmap_image` (also keeping the branch's `_rgba_nan_mask` helper right above it) - `quickflat.view.make_figure` (main's `Sequence[str]` for roi_list/sulci_list) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015yNgwu9miNKAuv79yPMqU8
#679 added two vertex attributes to the surface shaders, `flatBumpNorms` (vec3) and `flatheight` (float), for the bumpy flatmap. That put the 2D vertex program one over MAX_VERTEX_ATTRIBS, so it failed to link: THREE.WebGLProgram: gl.getProgramInfoLog() Too many attributes (mixNorms1) THREE.WebGLProgram: Could not initialise shader. WebGL: INVALID_OPERATION: drawElements: no valid shader program in use Vertex2D flatmaps then rendered blank and `save_3d_views` raised, which is gh-714. This branch had already spent the last free slot on `nanmask`, and folds the per-vertex alpha map into it for the same reason. Both of #679's attributes are static per-vertex geometry, computed together in one block of mriview_surface.js, so they pack into a single vec4 (.xyz the bump normal, .w the bump height) with no behavioural change and no per-dataset buffer churn. That returns one slot, which is all that was needed. `auxdat` has two free components but is deliberately left alone: it is static geometry, and per-dataset state there would mean rewriting the buffer on every setData. Verified on the headless viewer: a Vertex2D flatmap went from a link failure plus ~140 `no valid shader program in use` errors to none, and the three `vertex2d_*` cases in test_webgl_nan_alpha_parity.py plus the two Vertex2D cases in test_webgl_switching.py pass again. Those five passed before this branch merged main and regressed on the merge. The strict xfail #672 put on Vertex2D is removed: it was marked `raises=RuntimeError, strict=True` precisely so that a render which starts working reports rather than passing silently. Its reference images still have to be generated, along with a regeneration of the whole stored set for this branch's NaN/alpha changes; reference_images/README.md now says so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015yNgwu9miNKAuv79yPMqU8
#715 landed the same fix on main, and did it better. Both pack the bumpy flatmap's height into the fourth component of its normals, but #715 also moves the white matter and pial vertex areas into the two unused components of `auxdat`, which brings the worst case (2D vertex data on a subject with a flatmap) from 19 attributes down to 16 rather than to 17. That extra headroom matters: at 17, plain `Vertex` data still overflows as soon as equivolume sampling is on, which our fix left broken. #715 also names the packed attribute `flatbump`, factors its declaration into `utils.flatbump_attr`, and adds test_webgl_shaders.py, which links every shader variant the viewer can build and would have caught gh-714 in the first place. Dropping our version of the JS here so the merge of main takes #715's wholesale rather than resolving two equivalent edits line by line. The xfail removals and the reference-image bookkeeping are reconciled in the merge commit that follows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015yNgwu9miNKAuv79yPMqU8
main gained #715, which fixes gh-714 the same way this branch had just tried to and then some, so the JS took main's side wholesale (see the revert commit before this one). The two conflicts were both in the bookkeeping that fix came with: - test_visual_regression.py: the comment explaining why Vertex2D was xfailed. Both sides dropped the xfail; main's side wins, since #715 is what fixed it, not this branch. - reference_images/README.md: same, plus provenance. Took main's account of the four Vertex2D images and kept this branch's note that the flatmap references still need regenerating for the NaN/alpha changes. Left this branch's correction to the webgl/quickflat reference counts (18/14, not 16/12): #715 added four images without updating that line. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015yNgwu9miNKAuv79yPMqU8
Three gaps this branch's own changes left uncovered. test_webgl_shaders.py: #715's matrix already covers this branch's extra `nanmask` attribute -- all 27 variants still link, worst case included -- but not the options this branch adds to the volume-sampling shader. _nan_alpha_variants adds 20 surface_pixel variants over dataalpha x nanmean x layers (1 and 32) x the cmap/2d/rgb branches, hasflat and equivolume pinned on. 47 variants, still under seven seconds, and no subject database needed. test_visual_regression.py, the alpha map on 2D dataviews: _dataview grew a `twod_alpha` switch and NAN_ALPHA_DATAVIEW_NAMES now lists all four classes that take an explicit `alpha=`, not just the two RGB ones. Volume2D and Vertex2D accepted the argument before this branch as well, but it went into `attrs` as a bare ndarray, so quickflat mishandled it and the viewer failed to load the dataview -- that is the first fix in this PR and nothing in the visual suite could see it. _build_nan_dataview passes it too, which finally makes that builder do what its own comment says: NaN every place the rule names, the alpha map included. test_visual_regression.py, averaging across depth: a new multilayer_nan_dataviews suite, the three volumetric classes at both values of `nanmean`, with quickflat's `thick` and the viewer's `layers` both set to 32 and NaNs in diagonal slabs two voxels thick, so most surface points have both NaN and valid samples under them. The other suites NaN whole columns and leave depth sampling alone -- at their defaults the renderers do not even sample the same number of depths (quickflat 32, the viewer 1) -- so none of them can see the change this PR makes to how a column of samples is combined. Measured on renders from the regenerate path, the scenario separates cleanly and the renderers agree: data covers 0.797 of the flatmap with nanmean on and 0.196 with it off in quickflat, 0.815 and 0.185 in the viewer. The Volume2D case of that suite is flaky, and not because of anything here. Setting `layers` above 1 intermittently leaves the viewer's RPC proxy answering `{}` to every later query, so save_3d_views dies on the next parameter it sets; it never recovers and the browser reports no error. Building the same dataview inline and running the same sequence reproduces it on main at about the same rate, roughly one run in four, so it is a pre-existing viewer/JSProxy bug rather than a gh-695 regression -- this is just the first test to drive `layers` above 1 on a 2D dataview. Documented in the test and left in. References for all of this still have to be generated; see reference_images/README.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015yNgwu9miNKAuv79yPMqU8
|
While working on this, found #749. For now the test will xfail, but then we should decide how to handle it. |
236e8c9 added the multilayer and nan_alpha 2D cases but none of their reference images, so those suites could not run: _render_and_check_dataview fails outright on a missing reference. Adds the 16 that were absent (the 12 multilayer_nan_dataviews/ renders, and the four nan_alpha_dataviews/ ones for Volume2D and Vertex2D) and refreshes the 18 that had drifted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J5AMBzUzPKfDfVdjLvxqri
quickflat and the WebGL viewer average over the same number of depth samples here, but not at the same depths: quickflat takes the interior fractions linspace(0, 1, thick+2)[1:-1] while the shader takes i/(layers-1), which reaches both the white-matter and pial surfaces. On smooth data that is invisible -- every cross check outside this suite passes at mean|diff| 1.3-1.9 of 255 -- but the multilayer suite's NaN slabs are two voxels thick, at the sampling limit, so the offset flips which samples are NaN and the two renders disagree pixel by pixel. Five of its six parameter sets breach the tolerance. The disagreement is phase noise rather than a difference in what is drawn: signed bias within +-2, transparency agreeing to the same 1.19% outline as the passing suites, and mean|diff| dropping to 1.3 under a sigma=4 blur against a 1.18 floor. Both renderers also respond to nanmean the same way, which is what the suite exists to cover. Aligning the two grids would shift every render and invalidate both reference sets, so that is left to gh-749. Only the cross-renderer leg is conceded: the reference legs stay strict, and are asserted before the xfail so a real regression cannot hide behind it. Uses the imperative pytest.xfail rather than a mark, so the sixth parameter set -- which stays inside the tolerance -- simply passes, and all six will go green on their own once gh-749 is fixed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J5AMBzUzPKfDfVdjLvxqri
_build_nan_dataview NaN'd the vertex channels over three disjoint index ranges -- idx >= total/2, idx < total/4, and the quarter between -- which tile the surface exactly. A NaN in any channel renders an element transparent, so with no vertex left clean the two classes drawing on all three (Vertex2D, and VertexRGB through red/green/alpha) came out as bare curvature: 0% of their opaque pixels carried any color, against 10-100% everywhere else. The four nan_dataviews references for them pinned an empty flatmap, so they covered nothing -- a regression in 2D or RGB vertex NaN handling would still render blank and still pass, and the cross-renderer leg compared one blank flatmap against another. The volume regions never had this problem because they are three overlapping halves on independent axes (x>=50, y>=50, z>=15), whose union is 7/8 and leaves 37500 clean voxels. Splits the vertex regions on the vertex coordinates the same way, at the median of each axis so the masks stay at half the surface exactly. That leaves 39784 vertices (13.1%) clean, against 12.1% of the volume, and Vertex2D and VertexRGB now retain 13.07% all-finite elements rather than none. Volume regions are untouched, so only the four nan_dataviews vertex references need regenerating. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J5AMBzUzPKfDfVdjLvxqri
…d guard it Two follow-ups to the vertex masks. Leaving a remainder is not sufficient on its own: it also has to land somewhere the data can be seen. Vertex2D's visibility is governed by the alpha its 2D colormap derives from dim2, which is the accuracy bump, and >= on all three axes put the survivors off it -- mean accuracy 0.23, 3.5% of them above 0.5, and a render with 1.2% of its opaque pixels colored against Volume2D's 9.9%. Flipping the z comparison moves the remainder onto the bump (mean accuracy 0.62, 70% above 0.5) for the same 12% of the surface. VertexRGB improves with it; Vertex is unaffected, since it reads only the primary mask. Adds test_nan_builders_leave_clean_elements, asserting on the dataviews themselves that each NaN suite leaves at least 5% of elements free of NaNs in every channel -- the suites sit at 12% and above, so the floor is not tuned. Nothing could catch the original fault, because a blank render matches a blank reference exactly and agrees with the other renderer's blank render exactly as well; the failure is only visible before rendering. Checked against the masks as they were: it fails on Vertex2D and VertexRGB at 0.00% and passes the rest. The check needs no browser, so the playwright skip moves off pytestmark and onto the five render tests individually. Without playwright this file used to collect 39 skips and assert nothing; it now runs the 13 builder checks and skips the 26 renders. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J5AMBzUzPKfDfVdjLvxqri
|
@claude review |
|
Claude finished @mvdoc's task in 1m 47s —— View job Review
Everything new since the last pass is test/CI hardening plus one upstream merge. Summary: gh-714 shader fix, reverted in favour of upstream ( Test-quality fix worth calling out ( gh-749 xfail ( Minor, non-blocking note — the new Nothing here needs another round. Combined with the unchanged core logic (already reviewed and confirmed clean in the prior pass), this PR looks ready. |
WebApp.send waited 2 s for each answer and matched answers to requests by
arrival order only. When the browser was busy for longer (loading meshes,
or compiling the 32-layer Volume2D shader under software GL), the late
answer stayed in the queue and was read as the answer to the next request,
leaving every later request answered one behind for the rest of the
session. That is the intermittent Volume2D failure in
test_visual_comparison_multilayer_nan_dataviews: `run ui.set` got a stale
`{}` and the following query got `None`, so JSProxy.__getattr__ raised
"argument of type 'NoneType' is not a container".
Each request now carries an id, which python_interface.js echoes back, and
send() discards answers for other ids. JSProxy.__getattr__ also treats a
timed-out query (None) as retryable within max_time_retry instead of
crashing on it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGP6ytTVeCU29FbmuLNBza
This PR's new headless suites (the NaN/alpha visual-regression sets, the multilayer set, the quickshow-vs-WebGL parity and dataset-switching tests) add roughly ten minutes of webgl rendering, which puts the whole suite at 20-25 minutes on a hosted runner. The 3.10 job was killed at 79% with every test up to that point passing; 3.12 and 3.13 finished inside the limit on the same commit. The step timeout was only a guard against the viewer hanging, and pytest-timeout (pytest.ini, 240 s per test) already does that per test, so widening the step's backstop loses nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AGP6ytTVeCU29FbmuLNBza
Closes #629
Rule, applied identically in quickflat and the WebGL viewer: a NaN anywhere at a voxel/vertex (data, either dimension of a 2D view, any RGB channel, or the alpha map) renders fully transparent; otherwise the alpha (2D colormap,
alpha=kwarg, RGB alpha channel) is honored.Example
Fixes
Volume2D/Vertex2D(alpha=...): was an ndarray inattrs→ WebGL viewer failed to load (500); quickflat-Volume2D painted NaNs opaque, quickflat-Vertex2D ignored it. Now a proper attribute, multiplied into the colormap alpha, shipped to WebGL as a normalized brain (vertex data: folded into thenanmaskattribute — 2D vertex views already use 15/16 vertex attributes and extra ones made the program fail to link silently), and saved in HDF.VolumeRGB/VertexRGB: NaN mask was written into the temporary returned by.volumefor masked alpha Volumes (NaN voxels stayed opaque); single-frame alpha now broadcasts against multi-frame NaN masks (VertexRGB multiframe with NaN raises IndexError in alpha mask #629); NaN inside the alpha map → 0 explicitly;color_voxelsno longer mutates the caller's array.make_svgno longer indexesarr[..., 3]on 2-D images.nanmeantoggle in the surface controls (on by default); quickflat'snanmeandefaults toTruetoo, so both renderers agree. NaN-free data renders identically either way.Dataview2D.to_jsonkeepsvmin/vmaxof exactly 0.Packagededuplicates byte-identical brains (Vertex2D(x, x)crashed the viewer inreorder).NaNfor masked vertices instead of0.WebApp.sendwaited 2 s per answer and matched answers to requests by arrival order only. When the browser was busy for longer (loading meshes, compiling the 32-layer Volume2D shader under software GL), the late answer was read as the answer to the next request, and every request after it was answered one behind for the rest of the session. This was the intermittentTypeError: argument of type 'NoneType' is not a containerin the multilayer Volume2D visual-regression test. Requests now carry an id thatpython_interface.jsechoes back, and stale answers are discarded;JSProxy.__getattr__also retries a timed-out query instead of crashing on it.Tests
test_nan_alpha.py: browser-free unit tests for all of the above.test_webgl_nan_alpha_parity.py: 15 quickshow-vs-WebGL flatmap comparisons + the multi-layernanmeantoggle.test_webgl_switching.py: switching between datasets (setDataandaddData) never carries NaN masks or alpha over — checked for vertex/volume, scalar/RGB/2D.test_serve.py: browser-free tests that a late RPC answer is not read as the next one, and thatJSProxyretries after a timed-out query (both fail without the fix).CI
pytest-timeout(240 s,pytest.ini).Full suite green on CI for Python 3.10–3.14. Deliberately not touching the opacity slider (#685),
headless.py(#677) or reference images (#672).🤖 Generated with Claude Code
https://claude.ai/code/session_01KdjyZMFXsn6mRMJu9i8ige