Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The decimation examples discard the returned tractogram, and contributor documentation and one type annotation are inaccurate.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (4)
What changed in this PR
Adds tractography documentation, a synthetic gallery example, screenshot support for datasets, and visual regression coverage.
Changes:
- Documents tractogram loading, alignment, coloring, controls, and limitations.
- Extends headless 3D export to accept single-subject datasets.
- Adds three tractography visual regression scenarios.
| File | Description |
|---|---|
examples/tractography/README.txt |
Introduces the tractography gallery section. |
examples/tractography/plot_tractogram.py |
Demonstrates synthetic tract rendering and coloring. |
docs/tractography.rst |
Adds the tractography user guide. |
docs/index.rst |
Links the new guide. |
cortex/tests/test_webgl_tractogram.py |
Tests dataset subject resolution. |
cortex/tests/test_visual_regression.py |
Adds tract rendering regression tests. |
cortex/tests/reference_images/tracts/webgl_tracts_translucent.webp |
Adds translucent-surface reference. |
cortex/tests/reference_images/tracts/webgl_tracts_translucent_tracts.webp |
Adds translucent-tract reference. |
cortex/tests/reference_images/tracts/webgl_tracts_opaque.webp |
Adds opaque-surface reference. |
cortex/tests/reference_images/README.md |
Documents the new reference suite. |
cortex/export/save_views.py |
Supports datasets and custom surface filenames. |
cortex/export/headless.py |
Updates dataset support annotations. |
AGENTS.md |
Documents tractogram architecture and transport. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
4c6d171 to
96edb4c
Compare
|
What is failing — four tests, on every Python in the matrix: Why it is not this PR's. The base branch Root cause. dataviews = dataset.fromJSON({{data}});to metadata = {{data}};
dataviews = dataset.fromJSON(metadata);so that marker = "dataset.fromJSON("
start = page.index(marker) + len(marker)
return json.JSONDecoder().raw_decode(page, start)[0]That call now takes the identifier Fix (in - marker = "dataset.fromJSON("
+ marker = "metadata = "
start = page.index(marker) + len(marker)
return json.JSONDecoder().raw_decode(page, start)[0]
I have not pushed it, because the breakage is in #742 and the fix should land there so that PR merges green rather than carrying a red test into No re-run: the failure is deterministic, not a flake. Generated by Claude Code |
|
Fixed, in #742 rather than here: Verified before pushing: a viewer started with Generated by Claude Code |
e6ac083 to
c7cf36e
Compare
Streamlines bypass the mosaic and CTM machinery entirely. Package collects each Tractogram into four little-endian buffers -- points, offsets, colors and group membership -- served by a new TractHandler under /tract/, or written to tracts/*.bin by make_static. Colors arrive finished, since the colormapping happens in Python, so the browser side stays simple. resources/js/tractogram.js builds one THREE.Line in LinePieces mode per tractogram, with a Uint16 or Uint32 index depending on size and a duplicated-vertex fallback where OES_element_index_uint is missing. The controls live in their own #tracts panel under the dataset box rather than in dat.gui, because what a tractogram shows is data: a visibility checkbox and an opacity slider per tractogram, plus a checkbox per bundle when the file has groups. A streamline draws while it belongs to at least one checked group. Three things about Three.js r69 that this had to work around, each of which looked like a rendering bug first: - svgoverlay.js renders a depth pass with scene.overrideMaterial, which the line geometry cannot satisfy; tract objects opt out through userData.skipOverrideMaterial or r69 throws on the missing attributes. - r69 walks the transparent list backwards, so a translucent tractogram needs a large renderDepth to stay behind the surface rather than on top of it. - The material keeps writing depth at every opacity. Without it the streamlines have nothing to depth-test against each other and blend in buffer order, so the bundle last in the geometry paints over those in front and the apparent depth order changes as the opacity slider moves. Streamlines hide as soon as the surface starts to inflate or flatten, since their coordinates only mean anything against the folded surface, and a tractogram alone cannot open a viewer -- Package says so rather than failing obscurely later. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Dragging tract opacity to 0 did not hide the streamlines: it turned them into silhouettes cut out of the translucent surface. Writing depth at every opacity is what keeps the bundles from reordering themselves as the slider moves, but it also means fragments contributing no colour still hide whatever is behind them. r69's basic shader runs its alphatest discard while gl_FragColor.a is still just `opacity`, before vertex colours are folded in, so a small constant alphaTest drops exactly the fragments that would have been invisible anyway, depth write and all. Being constant, the ALPHATEST define compiles once and opacity changes stay cheap. The bundle checkboxes were in wire order, which for a real TRX is arbitrary: pyAFQ's HCP atlas lists IF0F_R, F_R, F_L, CC_ForcepsMajor and so on. They are now alphabetical, comparing digit runs as numbers so CST_2 precedes CST_10, with the synthetic "(ungrouped)" entry pinned last since it is not a bundle and its leading parenthesis would otherwise float it to the top. groupNames() returns the same order, so the panel and the API agree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`_served_metadata` located the dataset payload by the `dataset.fromJSON(`
marker and JSON-decoded from there. Adding the tract panel changed
mixer.html and static.html to assign the payload first, so that
`viewer.addTracts(metadata.tracts)` has something to read:
metadata = {{data}};
dataviews = dataset.fromJSON(metadata);
That call now takes an identifier rather than a JSON literal, so the
decode landed on `metadata);` and raised, failing the four tests that read
the served metadata:
json.decoder.JSONDecodeError: Expecting value: line 15603 column 30
The served page was always correct -- the viewer parses it fine -- so the
marker moves to the assignment. Line 4 of each template declares
`metadata` without an `=`, so the assignment is the page's first
`metadata = `.
Verified against a real served page: a viewer started with
open_browser=False reproduces the failure at exactly the line and column
CI reports, and returns the payload once the marker moves.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015zgY3aPBUWUhHBpisvF6Kw
A tractogram's name is a dataset key, i.e. an arbitrary string, and it was
reaching both the buffer urls and the file names of `make_static` verbatim:
`Dataset(**{"../../escape": tract})` wrote outside the output directory, and
any name holding a `/` failed outright. Buffers now travel under a transport
id (`cortex.webgl.data._tract_id`) that is safe as a single url segment and
as a file name; ordinary names pass through unchanged, so the urls stay
readable, and the metadata is still keyed by the name the user gave.
The viewer keys its tractograms by name in a plain object too, where a
tractogram called "constructor" or "toString" looked like one that was
already loaded -- addTracts would "replace" it and rmTracts would then trip
over the inherited value. Every lookup goes through `Viewer.hasTract` now,
and the per-group maps, whose keys are bundle names, are prototype-free.
Streamlines are meaningless against another subject's brain, but in a
multi-subject dataset they stayed on screen when the active dataview (and
with it the surface) switched subjects. A tractogram now shows only while a
dataview of its own subject is active, lines and panel entry alike; and
`Package` refuses at the outset a tractogram whose subject has no dataview
at all, rather than booting a viewer that silently omits it.
Tractograms load outside `viewer.loaded` on purpose, so that a slow
streamline download cannot hold up an otherwise usable viewer -- which left
`getImage`/`save_3d_views` free to screenshot a scene whose streamlines were
still in flight. `headless_viewer` waits for `viewer.tractsState()` on top of
`viewer.loaded`, and reports a failed download rather than waiting it out.
The panel's controls were bare inputs with no accessible name, and the
collapse control was a glyph in a <span>: unreachable by keyboard and
unannounced by a screen reader. They carry their own labels now, the
collapse and all/none controls are real buttons, and each bundle row is a
<label> wrapping its checkbox.
Testing: the index strategy is split into `mriview.indexModeFor`, so the
uint16/uint32/duplicate choice can be checked with plain arguments, and
`forceIndexMode` pins it, so the duplicated-vertex fallback is built and
compared segment by segment against the indexed geometry it replaces instead
of going untested for want of a 65536-point fixture. The headless test drops
its own polling loop, which the new wait makes unnecessary.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011GHv7KSxts6xnv1eQrgouT
`self.brains` and `self.images` are keyed by a BrainData's name, which is a hash of its array, so two distinct dataviews holding the same data are one brain on the wire. They were two entries in `uniques` all the same (BrainData compares by identity), which had `reorder` rewrite `self.images[name]` a second time on top of the npy blob its first pass had just written there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011GHv7KSxts6xnv1eQrgouT
- docs/tractography.rst: loading TRX, the millimeter space the points have to be in, the color modes, the viewer's tract panel, decimating a large tractogram, and what the feature cannot do (one-pixel lines, no flatmap, no HDF5). Linked from the user guide. - examples/tractography/plot_tractogram.py: three synthetic bundles on S1, rendered headlessly against a translucent cortex in both orientation and scalar coloring, so the gallery needs no diffusion data. - A tracts suite in the visual-regression tests: the same bundles against an opaque cortex, a translucent one, and a translucent one with translucent streamlines. The third is the one that earns its keep -- it is what fails if the streamlines stop occluding each other, which no tolerance on the other two would catch. - save_3d_views accepts a Dataset, since a Tractogram cannot be rendered alone and so never arrives as a lone dataview, and headless_viewer's annotation follows. A custom list_surfaces dict no longer gets stringified into the output filename. - AGENTS.md notes the container and the streamline transport. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review follow-ups on the tractography documentation: - `Tractogram.subsample` returns a decimated copy; the guide and the gallery example both called it for effect and threw the result away, so anyone following them would still have shipped the full tractogram to the browser. Both now assign it and say that the original is left untouched. - `_render_and_check_webgl_only` declared `angle: str` while the tract suite passes the `(name, parameters)` pair that `save_3d_views` also accepts. Both `angle` and `surface` are now typed with `ViewParams`, which gains the `surface_opacity` key the suite and the example set -- with that, the suite's own calls type-check where before they did not. - AGENTS.md said the TRX extra is imported lazily because CI's oldest Python is too old for it. `run_tests.yml` floors its matrix at 3.11 precisely so that `trx-python` installs; it is `install_from_wheel.yml` that still covers 3.10. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zgY3aPBUWUhHBpisvF6Kw
The two translucent-surface references were rendered before getTexture learned to undo the drawing buffer's premultiplication, so they carry colors that were faded twice: once by the fragment alpha and again by whatever composites the PNG. Against the current code they miss by mean|diff| 10.2 and 11.4, roughly half the pixels off by more than 16. Confirmed the premultiply correction is the only cause by disabling it, which makes all three tract references pass again. The regenerated pair changes nothing but color on translucent pixels: the alpha channel is identical, the 11306 fully opaque and 20227 fully empty pixels are byte-identical, and only the 54007 translucent ones move, getting brighter (mean luminance 53.2 -> 74.9) as undoing a premultiply should. webgl_tracts_opaque regenerates byte-identical and is left alone. Rendered on the pinned toolchain the references are provenanced to: chromium 151.0.7922.34 via playwright 1.62.0, matplotlib 3.10.9. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
c7cf36e to
d60a7e3
Compare
GitHub rebased tractography/3-webgl onto the updated tractography/2-dataview, so this branch still carried the pre-rebase copies of its five commits. Their patches are identical, so the merge keeps this branch's side of test_webgl_tractogram.py (the base copy plus the docs commits' additions). In the reference image README, takes main's commit-pinned WebP size measurement, since the reference count it replaced is stale now that main has Vertex2D images, and corrects the per-renderer reference counts to 21 webgl and 14 quickflat. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zki4QNnXMgNB6ZUL1PudF


Stack
Merge bottom-up; each PR is based on the one above it.
Documentation, a gallery example and visual references for the tractography
feature.
docs/tractography.rst: loading TRX, the millimetre space the pointshave to be in, the colour modes, the viewer's tract panel, decimating a large
tractogram, and what the feature cannot do — one-pixel lines, no flatmap, no
HDF5. Linked from the user guide.
examples/tractography/plot_tractogram.py: three synthetic bundles onS1, rendered headlessly against a translucent cortex in both orientation and
scalar colouring, so the gallery needs no diffusion data.
tractssuite in the visual-regression tests: the same bundles againstan opaque cortex, a translucent one, and a translucent one with translucent
streamlines. The third earns its keep — it is what fails if the streamlines
stop occluding each other, which no tolerance on the other two would catch.
Checked against the real renderer rather than assumed: reintroducing that
regression by hand moves 0.7% of the render past the gross-difference
threshold, against a 0.1% limit.
save_3d_viewsaccepts aDataset, since aTractogramcannot berendered alone and so never arrives as a lone dataview, and
headless_viewer's annotation follows. A customlist_surfacesdict nolonger gets stringified into the output filename.
AGENTS.mdnotes the container and the streamline transport.Testing
Three new LFS reference images, generated on the same Chromium and matplotlib
builds as the existing ones. Full suite green.
🤖 Generated with Claude Code