Enable 3D visualizations with CytoDataFrame#186
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds comprehensive 3D volume support: OME‑Arrow/TIFF volume extraction, per-row cropping and caching, vtk.js/PyVista/Trame rendering with interactive and static snapshot fallbacks, display options and examples, CellProfiler tutorial assets, tests, and related configuration/tooling updates. Changes
Sequence Diagram(s)sequenceDiagram
participant NB as Notebook
participant CDF as CytoDataFrame
participant VOL as VolumeModule
participant PV as PyVista
participant HTML as HTML/VTK.js
NB->>CDF: request _repr_html_()
CDF->>CDF: _find_3d_columns_for_display()
CDF->>CDF: check display_options (auto_trame_for_3d)
alt 3D detected & interactive allowed
CDF->>CDF: _get_3d_volume_from_cell(row,column)
CDF->>VOL: extract_volume_from_ome_arrow() or load file
VOL-->>CDF: (volume, dims)
CDF->>PV: _build_pyvista_viewer(volume,dims,...)
PV-->>CDF: viewer object / snapshot
CDF->>HTML: show_trame() or show_widget_table()
HTML-->>NB: interactive 3D display
else 3D detected but interactive unsupported
CDF->>PV: _pyvista_volume_snapshot_html()
PV-->>CDF: PNG data URL
CDF->>HTML: return img tag fallback
HTML-->>NB: static 3D preview
else No 3D
CDF->>HTML: standard 2D HTML table
HTML-->>NB: tabular display
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cytodataframe/frame.py (1)
1302-1330:⚠️ Potential issue | 🔴 CriticalCritical: Duplicate method definition for
_ensure_uint8.The static method
_ensure_uint8is defined twice - first at lines 1302-1312, then again at lines 1320-1330. The second definition shadows the first. The two implementations also have subtly different logic:
- First (line 1310):
if 0 <= min_val <= 255 and 0 <= max_val <= 255- Second (line 1328):
if min_val >= 0 and max_val <= 255Remove one of the duplicate definitions.
🐛 Proposed fix - remove the duplicate method
`@staticmethod` def _is_3d_image_array(array: np.ndarray) -> bool: return array.ndim >= 3 and not ( # noqa: PLR2004 array.ndim == 3 and array.shape[-1] in (1, 3, 4) # noqa: PLR2004 ) - - `@staticmethod` - def _ensure_uint8(array: np.ndarray) -> np.ndarray: - """Convert the provided array to uint8 without unnecessary warnings.""" - - arr = np.asarray(array) - if np.issubdtype(arr.dtype, np.integer): - min_val = arr.min(initial=0) - max_val = arr.max(initial=0) - if min_val >= 0 and max_val <= 255: # noqa: PLR2004 - return arr.astype(np.uint8, copy=False) - return img_as_ubyte(arr)
🤖 Fix all issues with AI agents
In `@pyproject.toml`:
- Around line 42-48: Remove the direct dependency on ipython-genutils from
pyproject.toml: delete the "ipython-genutils = \"^0.2.0\"" entry, then run your
dependency resolver (poetry/pip-tools/pip) to refresh the lockfile and ensure no
direct imports in the codebase require it; if any test/runtime failures surface,
identify the importing package and either add a maintained replacement or add a
compatible requirement with a comment, otherwise rely on the transitive
dependency resolution to supply it.
In `@src/cytodataframe/frame.py`:
- Around line 2968-2999: The nested _render_cell currently closes over the loop
variable image_col causing a late-binding bug; fix it by binding image_col per
iteration (e.g., turn the nested function into a renderer factory or give
image_col a default argument) so each _render_cell uses the correct column value
when called by data.loc[display_indices].apply; keep the existing calls to
_snapshot_cache_key, _get_3d_volume_from_cell and _pyvista_volume_snapshot_html
but ensure they use the bound image_col for that iteration.
In `@src/cytodataframe/volume.py`:
- Around line 274-279: The function extract_volume_from_ome_arrow declares an
ensure_uint8 parameter but never uses it; fix by applying ensure_uint8 to the
extracted numpy volume before returning (i.e., after you build the ndarray
inside extract_volume_from_ome_arrow call volume = ensure_uint8(volume) so the
returned ndarray is normalized to uint8), and keep the return type
Tuple[np.ndarray, Tuple[int,int,int]> intact; alternatively if normalization is
not desired remove the ensure_uint8 parameter and its type from the function
signature and all call sites.
- Line 178: The script src is unpinned
("script.src='https://unpkg.com/vtk.js';") which can break on upstream
changes—replace that string (and the other occurrence) with a pinned ESM package
URL for `@kitware/vtk.js` (e.g.
"https://unpkg.com/@kitware/vtk.js@<VERSION>/dist/vtk.js" or the appropriate ESM
entry), updating the literal assigned to script.src in the code paths that set
it (replace the "https://unpkg.com/vtk.js" literal wherever used, e.g., the
assignment to script.src at the two occurrences) and choose a concrete tested
<VERSION> before committing.
In `@tests/data/CP_tutorial_3D_noise_nuclei_segmentation/run.sh`:
- Around line 3-7: The CPDOCKER_RUNDIR variable points to the wrong directory
causing docker build to fail; update CPDOCKER_RUNDIR (used by the docker build
command alongside CPDOCKER_IMAGE_NAME) to the directory that contains the
Dockerfile (tests/data/CP_tutorial_3D_noise_nuclei_segmentation) so the docker
build -f "$CPDOCKER_RUNDIR/Dockerfile" . can locate the Dockerfile correctly;
ensure the docker build invocation remains unchanged.
🧹 Nitpick comments (11)
tests/data/CP_tutorial_3D_noise_nuclei_segmentation/Dockerfile (1)
1-50: Consider adding a non-root user for improved container security.The Dockerfile runs all operations as root. While acceptable for test/CI infrastructure, adding a non-root user is a security best practice that prevents potential privilege escalation if the container is compromised.
🛡️ Suggested improvement to add non-root user
WORKDIR /usr/local/src + +# Create non-root user for running cellprofiler +RUN useradd -m -s /bin/bash cpuser +USER cpuser + ENTRYPOINT ["cellprofiler"] CMD ["--run", "--run-headless", "--help"]tests/data/CP_tutorial_3D_noise_nuclei_segmentation/run.sh (1)
1-21: Add error handling and fix redundant command.The script lacks error handling (
set -e), which means failures will be silently ignored. Additionally, Line 21 specifiescellprofilerexplicitly, but since the ENTRYPOINT is alreadycellprofiler, this results in runningcellprofiler cellprofiler -c -r ....♻️ Proposed improvements
#!/bin/bash +set -euo pipefail # build and run cellprofiler from a docker container -CPDOCKER_RUNDIR=$PWD/src/docker/3d-nuclei-profiling +CPDOCKER_RUNDIR=$PWD/tests/data/CP_tutorial_3D_noise_nuclei_segmentation CPDOCKER_IMAGE_NAME=cp-3d-nuclei-profiling # build image docker build --platform linux/amd64 -t "$CPDOCKER_IMAGE_NAME" -f "$CPDOCKER_RUNDIR/Dockerfile" . # show the CellProfiler version and use run as a quick test echo "CellProfiler version:" docker run --rm --platform linux/amd64 -w /app \ -v "$CPDOCKER_RUNDIR:/app" \ "$CPDOCKER_IMAGE_NAME" \ --version # run cellprofiler with the examplehuman dataset from: # https://cellprofiler.org/examples docker run --rm --platform linux/amd64 -w /app \ -v "$CPDOCKER_RUNDIR:/app" \ "$CPDOCKER_IMAGE_NAME" \ - cellprofiler -c -r -p 3d-nuclei-profiling.cppipe -o output -i input + -c -r -p 3d-nuclei-profiling.cppipe -o output -i inputsrc/cytodataframe/volume.py (2)
145-150: Inconsistent transfer function configurations between renderers.The
build_3d_vtk_js_scriptandbuild_3d_vtk_js_initializeruse different color/opacity transfer functions:
- Script (lines 145-150): 2 RGB points (0→black, 255→white), 2 opacity points (0→0.0, 255→0.2)
- Initializer (lines 217-224): 3 RGB points (0→black, 1→white, 255→white), 3 opacity points (0→0.0, 1→0.15, 255→0.2)
This could cause different visual appearances when rendering the same volume via different code paths.
Consider extracting the transfer function configuration to a shared constant or function to ensure consistent rendering behavior.
Also applies to: 217-224
70-71: Potential in-place modification when input is already uint8.
volume.astype(np.uint8, copy=False)returns the same array if the dtype is already uint8, which meanstobytes()will serialize the original array. This is fine functionally, but be aware that if the volume is modified elsewhere concurrently, it could affect the serialization.For absolute safety with concurrent access, consider
copy=True, though the current implementation is acceptable for single-threaded use.tests/test_volume.py (1)
266-271: Consider removing unusednoqadirectives.The
# noqa: ANN204directives on__init__methods are flagged as unused by Ruff. These can be safely removed to reduce noise.♻️ Remove unused noqa directives
class FailingOMEArrow: - def __init__(self, data: str): # noqa: ANN204 + def __init__(self, data: str) -> None: raise RuntimeError("boom")Apply similar changes to
FakeOMEArrowandBadOMEArrowclasses.Also applies to: 290-295, 316-321
tests/test_frame.py (1)
1154-1154: Consider usingtmp_pathfixture instead of hardcoded/tmp.The test uses hardcoded
/tmppaths which triggers Ruff's S108 warning about insecure temporary file usage. While this is test code and poses minimal risk, using pytest'stmp_pathfixture would be more consistent with other tests in the file.♻️ Suggested improvement
- cdf = CytoDataFrame(base, data_context_dir="/tmp") + cdf = CytoDataFrame(base, data_context_dir=str(tmp_path))Add
tmp_path: pathlib.Pathto the test function signature.Also applies to: 1169-1169
src/cytodataframe/frame.py (5)
1862-1878: Silent exception swallowing hides potential issues.The
try-except-passblock at lines 1877-1878 silently swallows all exceptions from the OME-Arrow fallback path. This can mask import failures, data corruption, or other issues that would be useful for debugging.♻️ Proposed fix - add debug logging
try: from ome_arrow import OMEArrow # type: ignore if volume is None: ome_struct = OMEArrow(data=str(data_path)).data if hasattr(ome_struct, "as_py"): ome_struct = ome_struct.as_py() volume_data = extract_volume_from_ome_arrow( ome_struct, self._ensure_uint8, self._is_ome_arrow_value, logger, ) if volume_data is not None: volume, dims = volume_data - except Exception: - pass + except Exception as exc: + logger.debug("OME-Arrow fallback failed for %s: %s", data_path, exc)
1984-1995: Unused method argumentsdimsandwidget_width.The
dimsparameter (line 1987) andwidget_widthparameter (line 1990) are declared in the method signature but never used in the method body. The dimensions are recalculated fromvolume.shapeat line 2015, andwidget_widthis not referenced.Either use these parameters or remove them from the signature to avoid confusion.
♻️ Proposed fix - remove unused parameters
- def _build_pyvista_viewer( # noqa: C901, PLR0912, PLR0913, PLR0915 + def _build_pyvista_viewer( # noqa: PLR0913 self: CytoDataFrame_type, volume: np.ndarray, - dims: Tuple[int, int, int], backend: str, widget_height: str, - widget_width: str = "100%", spacing: Tuple[float, float, float] = (1.0, 1.0, 1.0), opacity: Any = "sigmoid", shade: bool = False, **kwargs: Any, ) -> Any:Then update all call sites to remove the
dimsargument.
2945-2951: Stub method_enqueue_snapshot_tasksdoes nothing.This method accepts
rowsandcolumnsparameters but immediately returnsNonewithout using them. If this is a placeholder for future implementation, consider adding a TODO comment or docstring explaining the intended behavior. If it's not needed, remove it.♻️ Proposed fix - add TODO or remove
def _enqueue_snapshot_tasks( self: CytoDataFrame_type, rows: List[Any], columns: List[Any], ) -> None: - """Queue background snapshot generation for given rows/columns.""" + """Queue background snapshot generation for given rows/columns. + + TODO: Implement background snapshot generation for improved performance. + Currently a no-op placeholder. + """ + # Future: implement async/threaded snapshot generation return None
2877-2877: Hardcoded spacing value instead of using variable.Line 2877 computes
base_sample = max(min((1.0, 1.0, 1.0)), 1e-6)which always evaluates to1.0. The hardcoded tuple(1.0, 1.0, 1.0)at line 2880 is assigned togrid.spacing, butbase_sampleshould likely use this same spacing variable rather than a separate hardcoded tuple.♻️ Proposed fix
+ spacing = (1.0, 1.0, 1.0) - base_sample = max(min((1.0, 1.0, 1.0)), 1e-6) + base_sample = max(min(spacing), 1e-6) grid = pv.ImageData() grid.dimensions = tuple(int(v) for v in vol_xyz.shape) - grid.spacing = (1.0, 1.0, 1.0) + grid.spacing = spacing
1314-1318: Consider removing unusednoqadirectives.The
noqa: PLR2004comments on lines 1316-1317 suppress magic number warnings, but static analysis indicates these directives are not needed (the rule may not be enabled). Consider removing them to keep the code clean.`@staticmethod` def _is_3d_image_array(array: np.ndarray) -> bool: - return array.ndim >= 3 and not ( # noqa: PLR2004 - array.ndim == 3 and array.shape[-1] in (1, 3, 4) # noqa: PLR2004 - ) + return array.ndim >= 3 and not ( + array.ndim == 3 and array.shape[-1] in (1, 3, 4) + )
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/cytodataframe/frame.py`:
- Around line 1801-1951: The method stores unbounded per-cell volumes in
self._custom_attrs["_volume_cache"], which can exhaust memory; change caching to
respect a display option and evict old entries: read display_options =
self._custom_attrs.get("display_options", {}) and if
display_options.get("volume_disable_cache") is truthy, skip storing/reading the
cache. Otherwise, initialize or replace _volume_cache with an ordered mapping
(e.g., collections.OrderedDict) keyed by cache_key and enforce a max size from
display_options.get("volume_cache_max_entries", 32) by popping the oldest entry
when inserting beyond the limit; update accesses around cache_key, cache[...] =
(volume,dims), and early-return on cache hit accordingly (symbols:
_volume_cache, cache_key, _custom_attrs, display_options).
🧹 Nitpick comments (3)
tests/test_frame.py (1)
1403-1416: Consider removing unusedmonkeypatchparameter.Static analysis correctly identifies that
monkeypatchis unused intest_enable_debug_mode_adds_handler_once. The test manipulates the logger handlers directly without needing monkeypatch.♻️ Suggested fix
-def test_enable_debug_mode_adds_handler_once(monkeypatch: pytest.MonkeyPatch) -> None: +def test_enable_debug_mode_adds_handler_once() -> None:src/cytodataframe/volume.py (1)
129-196: Consider extracting shared vtk.js rendering logic.
build_3d_vtk_js_scriptandbuild_3d_vtk_js_initializercontain nearly identical vtk.js setup code (ImageData creation, mapper/volume setup, renderer configuration, interactor binding). Consider extracting the shared rendering logic into a helper string constant or function to reduce duplication.♻️ Conceptual refactor
# Extract common vtk.js rendering code as a constant VTK_JS_RENDER_SETUP = ( "const imageData=vtk.Common.DataModel.vtkImageData.newInstance();" "imageData.setDimensions(dims);" # ... shared setup code ... "renderer.addVolume(volume);" "renderer.resetCamera();" "renderWindow.render();" ) def build_3d_vtk_js_script(element_id: str) -> str: return ( "<script>" "(function(){" f"const container=document.getElementById('{element_id}');" # ... per-element wrapper ... f"{VTK_JS_RENDER_SETUP}" # ... loading logic ... "})();" "</script>" )Also applies to: 199-270
src/cytodataframe/frame.py (1)
2834-2935: Consider silencing the unuseddimsparameter.If
dimsis intentionally unused, renaming to_dims(or using it for validation) will keep lint clean.♻️ Minimal rename
- def _pyvista_volume_snapshot_html( + def _pyvista_volume_snapshot_html( self: CytoDataFrame_type, volume: np.ndarray, - dims: Tuple[int, int, int], + _dims: Tuple[int, int, int], ) -> Optional[str]:
There was a problem hiding this comment.
Pull request overview
Adds first-class 3D volume visualization support to CytoDataFrame for notebook workflows, using VTK-in-HTML for inline rendering and a PyVista/trame-backed interactive layout option, while keeping existing 2D rendering behavior.
Changes:
- Introduces a new
cytodataframe.volumemodule to extract 3D volumes (notably from OME-Arrow) and generate 3D HTML/JS views. - Extends
CytoDataFramerendering to detect 3D content, optionally switch to a trame/PyVista-backed widget table, and provide snapshot fallbacks. - Expands tests, docs, and bundled test data to cover 3D pipelines and improve coverage reporting.
Reviewed changes
Copilot reviewed 19 out of 27 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Adds uv lock metadata (currently pins Python requirement). |
| src/cytodataframe/volume.py | New helpers for OME-Arrow volume extraction and VTK.js HTML/JS rendering. |
| src/cytodataframe/frame.py | Adds 3D detection/rendering paths, widget table + trame view integration, snapshot support, and path-resolution options. |
| pyproject.toml | Adds 3D visualization dependencies and pytest coverage configuration; reformats tool config. |
| poetry.lock | Updates lockfile to include new dependencies. |
| tests/test_volume.py | New unit tests for volume extraction + 3D HTML/JS helpers. |
| tests/test_frame.py | Adds extensive tests for 3D detection, widget/trame rendering paths, snapshots, and notebook environment detection. |
| tests/test_image.py | Adds/extends tests for outline drawing, bbox helper behavior, and scale bar edge cases. |
| tests/test_project_integration.py | Makes cosmicqc integration tests conditional via importorskip. |
| tests/data/CP_tutorial_3D_noise_nuclei_segmentation/run.sh | Adds script to regenerate 3D CellProfiler tutorial outputs (data provenance). |
| tests/data/CP_tutorial_3D_noise_nuclei_segmentation/Dockerfile | Adds a pinned Docker build environment for generating the tutorial outputs. |
| tests/data/CP_tutorial_3D_noise_nuclei_segmentation/3d-nuclei-profiling.cppipe | Adds the CellProfiler pipeline used to generate the tutorial outputs. |
| tests/data/CP_tutorial_3D_noise_nuclei_segmentation/output/MyExpt_Experiment.csv | Adds tutorial output metadata for 3D example/test workflows. |
| tests/data/CP_tutorial_3D_noise_nuclei_segmentation/output/MyExpt_Image.csv | Adds tutorial per-image CSV output. |
| tests/data/CP_tutorial_3D_noise_nuclei_segmentation/output/MyExpt_RealsizeNuclei.csv | Adds tutorial per-object measurements including 3D bbox fields. |
| tests/data/CP_tutorial_3D_noise_nuclei_segmentation/output/MyExpt_Watershed.csv | Adds tutorial object index/path metadata output. |
| docs/src/examples/cytodataframe_at_a_glance.py | Extends the example to include a 3D dataset workflow and visualization behavior. |
| docs/src/contributing.md | Notes that pytest output includes terminal coverage summary. |
| README.md | Documents default 3D notebook display behavior and configuration knobs. |
| CITATION.cff | Adds citations for 3D nuclei tooling/data used in examples. |
| .pre-commit-config.yaml | Updates hook versions and adjusts coverage hook invocation. |
| media/coverage-badge.svg | Updates coverage badge to reflect new coverage level. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cytodataframe/frame.py (1)
1741-1770:⚠️ Potential issue | 🟡 Minor
process_ome_arrow_data_as_html_display()return type isstr, but it can return a non-stringdata_value.Even if this is fine at runtime, it’s confusing (and undermines static typing). Converting the fallback to
str(data_value)would match the annotation.Proposed diff
if volume_data is None: - return data_value + return str(data_value) @@ except Exception: - return data_value + return str(data_value)
🤖 Fix all issues with AI agents
In `@pyproject.toml`:
- Around line 129-138: The TOML tables use invalid dotted-key layout; replace
the current [tool.pytest] with a proper table [tool.pytest.ini_options] and move
ini_options.addopts into the addopts key, and replace [tool.coverage] with
[tool.coverage.run] and move run.omit into the omit key (so use addopts =
"...cov..." and omit = [ "config.py", "config-3.py" ]), ensuring the sections
are named [tool.pytest.ini_options] and [tool.coverage.run] and the keys are
addopts and omit respectively.
In `@src/cytodataframe/frame.py`:
- Around line 1036-1043: find_image_columns() currently only treats strings as
potential paths (the lambda uses isinstance(value, str)), so Path objects are
missed; update the lambda in find_image_columns() to accept Path-like objects
(e.g., isinstance(value, (str, os.PathLike))) and call re.match on str(value) so
pathlib.Path or other PathLike values are handled; add the necessary import
(import os or from pathlib import Path) if missing.
- Around line 2190-2349: The backend parameter is annotated as backend: str =
"trame" but code accepts None (see backend is None checks and _repr_html_
calling with backend=None); update the type to allow None (e.g., backend: str |
None = "trame" or Optional[str] = "trame") in both show_trame and
show_widget_table signatures so the annotation matches actual usage; ensure
imports or Python version compatibility if you choose Optional[str].
- Around line 1833-1942: _get_3d_volume_from_cell is missing the CytoTable-style
path-column lookup and recursive rglob fallback that
_prepare_cropped_image_layers uses; add the same logic: when data_context_dir is
not set or the direct data_path.is_file() check fails, consult Image_PathName_*
columns (or the data_image_paths helper) for an alternative filename for the
same row, and if still not found perform a context_dir rglob search for the
filename to locate nested folders before attempting imageio or OME-Arrow
decoding; update _get_3d_volume_from_cell to normalize the candidate filename
the same way you do for 2D images, try candidates from the path-column list, and
replace the existing single-file lookup with a loop over discovered candidate
paths (including pathlib.Path(context_dir)/candidate and any rglob matches) so
volume/dims resolution mirrors _prepare_cropped_image_layers behavior.
🧹 Nitpick comments (10)
pyproject.toml (3)
8-11: Consider simplifying the multilinedescriptionto avoid accidental leading whitespace in published metadata.The current indentation inside the triple-quoted string can end up in the rendered package metadata (depending on how Poetry serializes it). This is mostly cosmetic, but it’s easy to avoid.
44-48: 3D stack deps: consider (a) adding upper bounds and (b) making them optional extras to avoid forcing VTK/Trame on all installs.Right now these are unconditional runtime deps with only lower bounds, which increases the chance of (1) install pain on some platforms and (2) breakage on future major versions.
Proposed diff (cap major versions; optionally mark as extras)
pyvista = ">=0.46.4" -trame = ">=3.12" -trame-vtk = ">=2.10" -trame-vuetify = ">=3.1" +trame = "^3.12" +trame-vtk = "^2.10" +trame-vuetify = "^3.1" nest-asyncio = "^1.6.0"If you want these optional (common for visualization stacks), Poetry-style could be:
# [tool.poetry.dependencies] pyvista = { version = "^0.46.4", optional = true } trame = { version = "^3.12", optional = true } trame-vtk = { version = "^2.10", optional = true } trame-vuetify = { version = "^3.1", optional = true } nest-asyncio = { version = "^1.6.0", optional = true } # [tool.poetry.extras] viz3d = ["pyvista", "trame", "trame-vtk", "trame-vuetify", "nest-asyncio"]
151-158:tasks.poster-render.shelllooks overly escape-heavy; consider switching back to a plain multiline shell block (like poster-preview).The current mix of
\\\n+ explicit line continuations is hard to maintain and easy to break.Proposed diff (clean multiline shell)
-tasks.poster-render.shell = """\ - quarto render \\\n docs/presentations/2025-sbi2/poster.qmd\n mv \ - docs/presentations/2025-sbi2/cytodataframe-2025-poster.pdf \\\n docs/src/_static/cytodataframe-2025-poster.pdf\n\ - """ +tasks.poster-render.shell = """ +quarto render \ + docs/presentations/2025-sbi2/poster.qmd +mv \ + docs/presentations/2025-sbi2/cytodataframe-2025-poster.pdf \ + docs/src/_static/cytodataframe-2025-poster.pdf +"""tests/test_frame.py (1)
823-862: Optional: clean up Ruff warnings in test helpers (unused args / unused noqa).A handful of inner helpers and lambdas intentionally ignore parameters; using
_/*_patterns (and removing now-unused# noqa) will keep the test file warning-free.Also applies to: 924-928
tests/test_volume.py (1)
127-141: Avoid hard-coding the vtk.js CDN URL/version in tests; import the constant fromcytodataframe.volumeinstead.This reduces maintenance friction when bumping vtk.js.
Proposed diff
-from cytodataframe.volume import ( +from cytodataframe.volume import ( + VTK_JS_CDN_URL, build_3d_html_from_path, build_3d_image_html_stub, build_3d_image_html_view, build_3d_vtk_js_initializer, build_3d_vtk_js_script, extract_volume_from_ome_arrow, ) @@ def test_vtk_js_helpers_include_expected_hooks(): script = build_3d_vtk_js_script("abc") initializer = build_3d_vtk_js_initializer() - assert "https://unpkg.com/@kitware/vtk.js@34.9.1/dist/vtk.js" in script + assert VTK_JS_CDN_URL in script assert "document.getElementById('abc')" in script assert "querySelectorAll('.cyto-3d-image[data-volume][data-dims]')" in initializersrc/cytodataframe/volume.py (2)
74-162: Consider validating(volume, dims)consistency to prevent hard-to-debug client-side failures.Since these helpers are public, a simple sanity check (3D volume,
np.prod(dims) == volume_uint8.size) would catch misuse early.Possible minimal validation
def build_3d_image_html_view( volume: np.ndarray, dims: Tuple[int, int, int], @@ ) -> str: @@ volume_uint8 = np.array(volume, dtype=np.uint8, copy=True) + if volume_uint8.ndim != MIN_VOLUME_NDIM: + return build_3d_image_html_stub( + data_value=data_value, + candidate_path=candidate_path, + display_options=display_options, + message="3D image has unsupported shape for inline rendering", + ) + if int(np.prod(dims)) != int(volume_uint8.size): + return build_3d_image_html_stub( + data_value=data_value, + candidate_path=candidate_path, + display_options=display_options, + message="3D image dims do not match volume size", + )
164-197: vtk.js init path: consider setting render-window size in the per-element script too (standalone HTML robustness).Right now
build_3d_vtk_js_script()usesinclude_container_size=False, so standalone HTML that relies on the embedded<script>may render at an incorrect/zero size depending on vtk.js behavior. Using container-based sizing in both paths tends to be more reliable.Proposed diff (enable sizing in the per-element script)
def build_3d_vtk_js_script(element_id: str, vtk_js_url: Optional[str] = None) -> str: @@ - f"{_build_vtk_js_renderer_core(include_container_size=False)}" + f"{_build_vtk_js_renderer_core(include_container_size=True)}"Also applies to: 200-247, 249-284
src/cytodataframe/frame.py (3)
1431-1474: 3D-in-_prepare_cropped_image_layers: consider applying the XY bounding box crop before building the inline volume HTML.Without cropping, inline volume payloads can be much larger than necessary and may exceed
max_inline_volume_byteseven when the per-cell crop would fit.
2020-2048: Optional:_find_3d_columns_for_display()swallows all exceptions; consider a debug log when a candidate column fails probing.A single debug log (maybe gated behind a display/debug option) can make “why didn’t auto-trame trigger?” much easier to diagnose.
3036-3115:_generate_trame_snapshot_html()has a broadexcept Exception: passin the cell renderer; consider logging at debug at least once.Even a minimal
logger.debug(..., exc_info=True)would help debug “Snapshot unavailable” cases without spamming normal logs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.pre-commit-config.yaml:
- Around line 56-60: Update and verify the pre-commit bump to ruff rev "v0.15.0"
by running the new formatter and checker locally: run `ruff format` across the
repo (or via the ruff-format hook) to reformat the entire codebase to the 2026
style and commit those changes, then run `ruff check` (or ruff-check hook) to
surface lint failures; also validate config resolution for pyproject.toml and
any extended configs by running ruff with `--show-settings`/`--config` as needed
to ensure the updated resolver loads expected options; reference the hooks
ruff-format and ruff-check and the rev: "v0.15.0" entry when making and testing
these changes.
jenna-tomkinson
left a comment
jenna-tomkinson
left a comment
There was a problem hiding this comment.
LGTM! Super excited to see this!
Co-Authored-By: Jenna Tomkinson <107513215+jenna-tomkinson@users.noreply.github.com>
Co-Authored-By: Jenna Tomkinson <107513215+jenna-tomkinson@users.noreply.github.com>
Co-Authored-By: Jenna Tomkinson <107513215+jenna-tomkinson@users.noreply.github.com>
Co-Authored-By: Jenna Tomkinson <107513215+jenna-tomkinson@users.noreply.github.com>
|
Thanks @jenna-tomkinson for the review! Merging this in. |
2 participants
Description
This PR enables 3D visualizations with CytoDataFrame. We use
trameandpyvistato assist with this to provide interactivity within Jupyter notebooks for 3D objects. Existing 2D functionality remains rendered throughipywidgets.Note: this only addresses raw image viewing. We'll follow up with labels (masks, outlines) in separate work.
Closes #175
What kind of change(s) are included?
Checklist
Please ensure that all boxes are checked before indicating that this pull request is ready for review.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores