-
Notifications
You must be signed in to change notification settings - Fork 158
CI add visual regression tests #672
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
ac11d8d
0ee0b0b
b730a29
dcd4c24
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| cortex/tests/reference_images/**/*.webp filter=lfs diff=lfs merge=lfs -text |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -99,6 +99,35 @@ def _wait_for_viewer_loaded(handle, timeout: float = 60.0) -> None: | |
| # --------------------------------------------------------------------------- # | ||
|
|
||
|
|
||
| #: How often the worker thread calls into Playwright to dispatch queued browser | ||
| #: events; ``browser_errors`` is current to within this interval. | ||
| EVENT_PUMP_INTERVAL = 0.25 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is "EVENT PUMP" standard terminology in this pattern or can we rename it to something more intuitive? |
||
|
|
||
| #: Browser messages that mean WebGL itself failed, as opposed to unrelated | ||
| #: javascript a page may log. Deliberately narrow: a healthy viewer already logs | ||
| #: a console.error for the Leap Motion websocket it cannot reach | ||
| #: (ws://127.0.0.1:6437), so asserting on any error at all fails every run. | ||
| #: | ||
| #: A link failure arrives as a console.error rather than an exception, so | ||
| #: nothing raises and the render comes back blank -- how Vertex2D broke | ||
| #: (gh-714). three.js emits it alongside "gl.VALIDATE_STATUS false" and | ||
| #: "gl.getError() 0"; do not add those. Nothing here calls gl.validateProgram(), | ||
| #: so VALIDATE_STATUS is false for want of a run, and getError() 0 is the | ||
| #: absence of an error. Driver shader-info warnings are excluded likewise. | ||
| WEBGL_FAILURE_PATTERNS = ( | ||
| "THREE.WebGLProgram: Could not initialise shader", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we do a final sweep and use American spelling instead of British?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or is the error itself with british spelling? did we create that error type, or three.js?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The error is from three.js and has British spelling. Verbatim from the chromium console:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll fix the spellings elsewhere. Maybe we should add something in |
||
| "Error creating WebGL context", | ||
| ) | ||
|
|
||
|
|
||
| def filter_webgl_failures(browser_errors: list[str]) -> list[str]: | ||
| """Return only those browser messages that indicate WebGL itself failed.""" | ||
| return [ | ||
| e for e in browser_errors | ||
| if any(pattern in e for pattern in WEBGL_FAILURE_PATTERNS) | ||
| ] | ||
|
|
||
|
|
||
| class _PlaywrightThread: | ||
| """Manages the Playwright lifecycle on a private daemon thread. | ||
|
|
||
|
|
@@ -230,7 +259,19 @@ def _worker(self) -> None: | |
| return | ||
|
|
||
| # Keep the thread (and therefore Playwright) alive until shutdown. | ||
| self._shutdown_event.wait() | ||
| # | ||
| # Playwright's sync API dispatches queued events only while something is | ||
| # calling into it, so parking here for the viewer's lifetime would leave | ||
| # console messages undelivered until _cleanup(). Poll instead: the cheap | ||
| # round-trip pumps the loop. | ||
| while not self._shutdown_event.wait(EVENT_PUMP_INTERVAL): | ||
| try: | ||
| self._page.evaluate("0") | ||
| except Exception: | ||
| # Usually the page closing during shutdown, but anything else | ||
| # ending the pump silently stops browser_errors updating. | ||
| logger.debug("event pump stopped", exc_info=True) | ||
| break | ||
| self._cleanup() | ||
|
|
||
| # -- Playwright event handlers (called on the worker thread) ---------- # | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,137 @@ | ||
| # Reference images | ||
|
|
||
| Stored renders that `cortex/tests/test_visual_regression.py` asserts against. | ||
|
|
||
| ## Contents | ||
|
|
||
| | directory | images | contents | | ||
| | --- | --- | --- | | ||
| | `alpha_dataviews/` | 10 | five of the six public dataview classes (`Volume`, `Vertex`, `Volume2D`, `VolumeRGB`, `VertexRGB`), both renderers | | ||
| | `nan_dataviews/` | 10 | the same five, with NaNs over roughly half the primary data channel | | ||
| | `nan_alpha_dataviews/` | 4 | `VolumeRGB`/`VertexRGB` only, with the NaNs in the `alpha=` map | | ||
| | `nonflat_views/` | 4 | `Volume`/`Vertex` on the inflated and fiducial surfaces at `lateral_pivot`, webgl only | | ||
|
|
||
| Filenames are `quickflat_<Class>` and `webgl_<Class>`, except `nonflat_views/`, | ||
| which uses `webgl_<surface>_<angle>_<Class>`. | ||
|
|
||
| `Vertex2D` is the sixth class and has no images: its webgl flatmap renders | ||
| blank (gh-714) and `save_3d_views` raises, so it cannot be tested through the | ||
| webgl path at all. The two `Vertex2D` tests are **xfailed** on that | ||
| `RuntimeError`, strictly — if the render ever succeeds the XPASS says so rather | ||
| than passing silently. | ||
|
|
||
| ## Render settings | ||
|
|
||
| The three flatmap directories render `quickflat_*` with `cortex.quickshow` and | ||
| `webgl_*` with `cortex.export.plot_panels`, both with curvature | ||
| **un-thresholded** (`curvature_threshold=False` and | ||
| `surface.{subject}.curvature.smoothness=1.0`). (This is to avoid failures from | ||
| differences in the renderers' anti-aliasing implementations.) | ||
| Everything else is at its default. | ||
|
|
||
| `nonflat_views/` calls `save_3d_views` directly, so the stored pixels are the | ||
| browser screenshot with its transparent background, not a matplotlib | ||
| composition of it. It keeps pycortex's default thresholded curvature. | ||
|
|
||
| The exact keyword arguments are in `_render_and_check_dataview` and | ||
| `_render_and_check_webgl_only`; change either and the references must be | ||
| regenerated. | ||
|
|
||
| ## Checks | ||
|
|
||
| The three flatmap tests check each render twice: against its own stored | ||
| reference at a tight tolerance (`MAX_MEAN_ABS_DIFF`, `MAX_FRACTION_DIFFERING`, | ||
| `MAX_FRACTION_GROSSLY_DIFFERING`, `MAX_SSIM_LOSS`, all four of which must pass), | ||
| and against the other renderer's render of the same dataview at a loose one | ||
| (`CROSS_MAX_MEAN_ABS_DIFF`, `CROSS_MAX_FRACTION_DIFFERING`), with no stored | ||
| fixture. `test_visual_comparison_nonflat_views` runs the reference check only. | ||
|
|
||
| ## The cross-renderer affine correction | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is fancy and if it works, fine. but wouldn't have it worked to just crop both images tightly, then match pixel size? |
||
|
|
||
| The renderers are not perfectly aligned pixel-by-pixel. Before diffing, | ||
| `_check_cross_renderer` maps webgl onto quickflat's frame with a fixed | ||
| anisotropic scale (x 0.9690, y 0.9365) and a ~7 px translation, held in the | ||
| `CROSS_RENDERER_*` constants in `cortex/tests/test_visual_regression.py`. Over | ||
| the twelve stored pairs it takes mean|diff| from 4.83-9.42 down to 1.06-2.01. | ||
|
|
||
| The correction is affine, not perspective, despite webgl rendering through a | ||
| `THREE.PerspectiveCamera` (FOV 45°, `axes3d.js`): the flatmap is a planar | ||
| surface viewed down its normal, so the projection degenerates to a scale. | ||
| Fitting a full homography returns a projective row of `[~0, ~0, 1]` and ~0° | ||
| rotation. Do not try to undo a perspective projection here. | ||
|
|
||
| To re-derive the constants: | ||
|
|
||
| ``` | ||
| uv run --with opencv-python-headless \ | ||
| python cortex/tests/reference_images/fit_cross_renderer_affine.py | ||
| ``` | ||
|
|
||
| It renders curvature-only content, fits webgl → quickflat with | ||
| `cv2.findTransformECC`, and prints the constants ready to paste. OpenCV is not a | ||
| project dependency, hence `--with`. It also writes `fit_affine_residual.png`, | ||
| which should show a faint sulcal outline only. | ||
|
|
||
| Re-run it after a change to `plot_panels`' figure composition, to quickflat's | ||
| `height=`/`dpi=`, or to either renderer's trim logic. Losing the correction | ||
| looks like a mean|diff| around 5-9 rather than 1-2 across most pairs — 11 of the | ||
| 12 fail, so one green pair is not evidence the correction is intact. Its | ||
| geometry settings (`height`, `dpi`, `figsize`, `windowsize`) mirror the test's | ||
| and must be updated alongside them; its curvature settings deliberately do not | ||
| (see the note in the script). | ||
|
|
||
| ## Provenance | ||
|
|
||
| Generated on `main` (`3779f7ca`). | ||
|
|
||
| | | | | ||
| | --- | --- | | ||
| | chromium | 151.0.7922.34 (headless shell, SwiftShader software rendering) | | ||
| | playwright | 1.62.0 (fixes the chromium build above) | | ||
| | matplotlib | 3.10.9 | | ||
|
|
||
| Both are pinned in the `test` dependency group, and re-pinning is part of | ||
| regenerating. playwright fixes the chromium build; matplotlib rasterises the | ||
| quickflat renders and `plot_panels`' composition of the webgl ones, so between | ||
| them they determine all 28 images. | ||
|
|
||
| Update matplotlib beyond 3.10.9 once Python 3.10 is dropped. | ||
|
|
||
| ## Format | ||
|
|
||
| Lossless WebP (`method=6`, `quality=100`, `exact=True`): bit-exact after decode, | ||
| and 59% the size of optimized PNG (1229 KiB versus 2061 KiB for the set of 28). | ||
|
|
||
| ## Storage | ||
|
|
||
| Tracked with **git LFS**. If yours are 130-byte text files rather than images, | ||
| the clone has not fetched them: | ||
|
|
||
| ``` | ||
| git lfs install && git lfs pull | ||
| ``` | ||
|
|
||
| The tests skip on that, and on the images being absent altogether, rather than | ||
| failing. | ||
|
|
||
| ## Distribution | ||
|
|
||
| Kept out of the wheel (`exclude_package_data` in `setup.py`) and kept in the | ||
| source tarball (`MANIFEST.in`'s `recursive-include cortex *`), so a run against | ||
| an installed wheel degrades gracefully. | ||
|
|
||
| ## Regenerating | ||
|
|
||
| The renders are deterministic: repeated runs on one machine produce | ||
| bit-identical output, including the WebGL ones under software rendering. They | ||
| are coupled to the Chromium and matplotlib builds above, so an upgrade can shift | ||
| anti-aliasing and rasterization; the tolerances absorb small shifts. If a | ||
| failure exceeds them, inspect the `diff_*.png` files it writes, confirm the | ||
| change is cosmetic, then: | ||
|
|
||
| ``` | ||
| REGENERATE_REFERENCE_IMAGES=1 pytest cortex/tests/test_visual_regression.py | ||
| ``` | ||
|
|
||
| That rewrites all four directories in one run. It does not refit the affine | ||
| correction. Review the resulting diff before committing. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd remove this comment, or at least reword it. "No lfs: true on purpose" is confusing.