Skip to content

FIX unify NaN and alpha handling across quickflat, WebGL and RGB dataviews - #695

Open
mvdoc wants to merge 19 commits into
mainfrom
fix/nan-alpha-parity
Open

mvdoc wants to merge 19 commits into
mainfrom
fix/nan-alpha-parity

Conversation

@mvdoc

@mvdoc mvdoc commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

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

nan_alpha_matrix

Fixes

  • Volume2D/Vertex2D(alpha=...): was an ndarray in attrs → 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 the nanmask attribute — 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 .volume for 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_voxels no longer mutates the caller's array.
  • quickflat averages RGBA across thickness in premultiplied space (no dark halos around transparent voxels; matches WebGL). make_svg no longer indexes arr[..., 3] on 2-D images.
  • NaN averaging across depth: the WebGL multi-layer sampling summed layer samples, so one NaN voxel at any depth hid the whole fragment. It now averages only the valid layers (transparent only if none is valid), with a nanmean toggle in the surface controls (on by default); quickflat's nanmean defaults to True too, so both renderers agree. NaN-free data renders identically either way.
  • Dataview2D.to_json keeps vmin/vmax of exactly 0.
  • Package deduplicates byte-identical brains (Vertex2D(x, x) crashed the viewer in reorder).
  • Hover/click readout shows NaN for masked vertices instead of 0.
  • Viewer RPC desync (pre-existing, also on main): WebApp.send waited 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 intermittent TypeError: argument of type 'NoneType' is not a container in the multilayer Volume2D visual-regression test. Requests now carry an id that python_interface.js echoes 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-layer nanmean toggle.
  • test_webgl_switching.py: switching between datasets (setData and addData) 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 that JSProxy retries after a timed-out query (both fail without the fix).

CI

  • The pytest step timeout goes from 25 to 40 minutes: the new headless suites add about 10 minutes of WebGL rendering, and the suite now takes 30+ minutes on hosted runners. Hangs are still caught per test by 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

mvdoc and others added 6 commits August 20, 2026 17:59
…#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
@mvdoc
mvdoc requested review from evi-hendrikx and a balanced review from Copilot August 21, 2026 17:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 nanmean control.
  • 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.

Comment thread cortex/quickflat/utils.py
Comment thread cortex/webgl/resources/js/shaderlib.js Outdated
Comment thread cortex/webgl/resources/js/dataset.js Outdated
Comment thread cortex/dataset/viewRGB.py Outdated
Comment thread cortex/webgl/data.py Outdated
… 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
@mvdoc

mvdoc commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the Copilot review in the latest commit:

  1. quickflat nanmean ignored for RGBA dataviews — 2D/raw conversions now pass their NaN mask through to make_flatmap_image, which renormalizes over valid voxels (nanmean=True) or hides any pixel touched by a NaN voxel (nanmean=False). Native RGB has no NaN left after conversion, so fully transparent voxels count as missing there — the same rule as the WebGL RGB textures.
  2. NANMEAN missing for RGB textures — added to the RGBCOLORS branch of surface_pixel (transparent layer samples are skipped; validity from the current frame, since an unbound next-frame sampler reads as opaque black).
  3. Vertex mask only from fframe — both blended frames are now AND-ed for every dim and the alpha map (the next frame only while framemix > 0).
  4. alpha.vmin sentinel for uint8 alpha — uint8 alpha maps are filled with byte 0.
  5. Dedup by brain.name alone — same-name brains are merged only when class/subject/xfm/shape/dtype/mask match, otherwise Package raises a clear ValueError. The full "separate package IDs + rewritten view references" redesign is out of scope here; the previous behavior silently let the last same-name brain win.

Each has a test (test_nan_alpha.py, test_webgl_data.py, test_webgl_nan_alpha_parity.py).

@mvdoc

mvdoc commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

@claude review

@claude

claude Bot commented Aug 28, 2026

Copy link
Copy Markdown

Claude finished @mvdoc's task in 5m 3s —— View job


Review

  • Gather context (diff vs actual merge-base 0ceec8c, matches PR's reported +1666/-116 across 17 files)
  • Review latest fixup commit (814e53b) addressing Copilot's 5 comments
  • Review remaining files for correctness, edge cases, tests
  • Post final review

Note: I couldn't execute python/pytest in this sandbox (all non-git Bash commands required approval I wasn't able to grant), so this is a static read-through, not a verified test run. If you'd like me to actually run the suite, allow Bash(python3:*)/Bash(pytest:*) in --allowedTools.

Copilot's 5 findings — all look correctly fixed in 814e53b

  1. quickflat nanmean ignored for RGBA (quickflat/utils.py): now threads a NaN-validity mask (_rgba_nan_mask) separately from alpha through the raw conversion, and renormalizes the premultiplied average by the valid-voxel weight (avg / w_valid) rather than the total pixmap weight — that's the correct fix, not just a cosmetic one, since pixmap rows are pre-normalized weights and dividing by the valid-only weight sum is exactly nanmean semantics. nanmean=False now correctly zeroes any pixel touched by a NaN voxel via the w_nan check.
  2. NANMEAN missing for RGB textures (shaderlib.js): the RGBCOLORS branch now skips fully-transparent (c0.a > 0.) layer samples and normalizes by nvalid instead of a fixed 1/layers, with nvalid folding back to layers when NANMEAN is off — so the non-NaN case renders identically either way.
  3. Vertex mask only from fframe (dataset.js): pushFrames now ORs in the next frame's mask too, but only while fmix > 0 (actually blending toward it) — avoids paying the cost when not needed, while fixing the movie-interpolation bug.
  4. uint8 alpha vmin sentinel (viewRGB.py): _mask_alpha now special-cases dtype == np.uint8 to fill with byte 0 rather than trusting alpha.vmin (which for uint8 is a percentile of the raw bytes, e.g. 255 for a constant map).
  5. Dedup by name alone (webgl/data.py): _brain_signature now compares type, subject, xfmname, data shape/dtype and mask hash before merging same-name brains, raising a clear ValueError otherwise. I checked the one gap I could think of — RGB composite views (VolumeRGB/VertexRGB) don't expose .data/._mask so their signature degrades to (type, subject, xfmname) — but their .name is a content hash of .volume/.vertices, which already bakes in each channel's own mask, so a same-name collision with a different mask can't actually happen there; the added test_package_rejects_same_bytes_different_metadata test covers the direct case well.

Each fix has good comments explaining why (not just what), and each is covered by a new/extended test.

Rest of the diff

  • view2D.py: the new alpha property/_normalized_alpha/_wrap_alpha machinery is clean; raw() correctly extends the existing "same mask ⇒ linear path" check to also require the alpha map share dim1's mask before taking the faster linear branch. The vmin/vmax is None checks (replacing or) correctly fix the "legitimate 0" bug consistent with the base Dataview.to_json fix.
  • viewRGB.py: _mask_alpha cleanly unifies the masked/linear/full-volume broadcasting cases and documents the "never mutate a temporary" fix. color_voxels no longer mutates a caller-supplied alpha array. nan_to_num before the uint8 cast is a good defensive fix for undefined-behavior-on-cast.
  • quickflat/view.py make_svg: now guards arr[..., 3] behind arr.ndim == 3 and floating, correctly skipping 2-D scalar images.
  • shaderlib.js: the DATAALPHA/NANMEAN preprocessor wiring is consistent between the single-sample and multi-layer paths, and the vertex shader documents why alpha is folded into nanmask instead of a new attribute (16-attribute cap).
  • Docs and the example script accurately describe the new behavior and match what the code does.

Minor, non-blocking observations

  • view2D.py Volume2D.raw()/Vertex2D.raw() no longer forward **self.attrs to the constructed VolumeRGB/VertexRGB (only priority is passed explicitly now). Previously **self.attrs was forwarded wholesale (which is exactly what caused the original "ndarray alpha stuck in attrs" bug for alpha), so dropping it is probably intentional, but if users rely on any other custom kwarg surviving into .raw, that would now silently disappear. Worth a sanity check but I don't see evidence any other attrs currently matter here.
  • The JS mouseover NaN-readout fix (mriview.js) only ever reads nanmasks[0]/verts[0] (frame 0), same as the pre-existing verts[0] readout — consistent, not a regression, just noting the hover readout isn't movie-frame-aware in general (pre-existing behavior, out of scope here).

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
@mvdoc

mvdoc commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

While working on this, found #749. For now the test will xfail, but then we should decide how to handle it.

mvdoc and others added 5 commits September 21, 2026 13:51
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
@mvdoc

mvdoc commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

@claude review

@claude

claude Bot commented Sep 21, 2026

Copy link
Copy Markdown

Claude finished @mvdoc's task in 1m 47s —— View job


Review

  • Gather context (diff vs origin/main, matches PR's reported +2164/-187 across 56 files)
  • Review commits added since the last review pass (all core production code — view2D.py, viewRGB.py, quickflat/*, webgl/data.py, dataset.js, mriview.js, shaderlib.js apart from the merged-in upstream fix — is unchanged since then; confirmed with git diff c3ac88a..HEAD -- cortex/dataset cortex/quickflat cortex/webgl/data.py docs examples, which is empty. So the previous review's assessment of that code still stands, and Copilot's 5 findings remain correctly fixed.)
  • Check the new xfail for quickflat and the WebGL viewer sample cortical depth on different grids #749 and the test cleanup commits
  • Post final 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 (edcefc0/60a2515/434aaa7) — the branch's own fix for the flatbump-attribute overflow was cleanly dropped once #715 landed the same idea on main with more headroom (16 attributes worst case vs. 17). Good call to take the upstream version wholesale rather than reconcile two equivalent edits; the merge is clean and test_webgl_shaders.py (from #715) now also exercises this branch's own new dataalpha/nanmean shader options (236e8c9, 20 extra variants).

Test-quality fix worth calling out (7b7ac47/d98e962) — real bug found in the test suite itself: _build_nan_dataview's three vertex NaN masks were disjoint index ranges that exactly tiled the whole surface, so Vertex2D and VertexRGB (which read all three) had every vertex NaN'd somewhere and rendered as bare curvature. Because both the reference and cross-renderer checks compare against/against-each-other renders that are equally blank, nothing caught it. Fixed by switching to overlapping coordinate-based masks (mirroring the existing volume regions) tuned so the ~12% clean remainder actually lands on visible data, plus a new browser-free test_nan_builders_leave_clean_elements that asserts every NaN suite leaves ≥5% NaN-free elements — specifically to guard against this class of silent-pass failure going forward. This is exactly the kind of self-referential test bug that's easy to miss and good to see caught and guarded against.

gh-749 xfail (7cfc997) — narrowly scoped: only the multilayer suite's cross-renderer leg is conceded, via imperative pytest.xfail (not a mark) so the one parameter set that already agrees just passes, and the reference legs are asserted before the xfail so a real regression can't hide behind it. The root cause (quickflat's interior depth-sample grid vs. the shader's edge-inclusive grid, only visible at the two-voxel NaN slab thickness this suite uses) is well-documented and reasonably deferred to a separate issue rather than shifting every reference image to align the two grids.

Minor, non-blocking note — the new multilayer_nan_dataviews Volume2D case is documented as intermittently flaky (~1 in 4) under headless Chromium due to a pre-existing JSProxy/viewer RPC issue reproducible on main, not a regression from this PR. It's left in and documented rather than skipped; worth keeping an eye on in CI but not a reason to hold this up.

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

This branch has not been deployed

No deployments
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.

VertexRGB multiframe with NaN raises IndexError in alpha mask

4 participants