From 7ccb88fee907cc557fb500934ef4daa12799f0fa Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Tue, 25 Aug 2026 21:44:32 -0600 Subject: [PATCH 1/4] Organize and upload images referenced by NWB files with ExternalImage `ExternalImage` holds a path in `data` the way `ImageSeries` holds a list in `external_file`, but nothing in the CLI knew about it: `dandi organize --update-external-file-paths` did the whole job for videos and none of it for images, and `dandi upload` dropped a `.png` as an unrecognized file. `DandiFileType.classify` now returns `IMAGE` for the extensions NWB allows an external image to be, which makes them `ImageAsset` rather than `GenericAsset` and so uploadable without a flag. `_get_external_images` collects `ExternalImage` objects by walking `nwb.objects`, since they sit inside an `Images` container rather than at the top of a module, and tags its entries `field="data"` so the existing `ImageSeries` ones are tagged `field="external_file"` and both flow through the same naming and relocation code. `_rename_external_images` writes the rewritten path directly to the dataset, because `data` is a scalar string and assigning through the read container does not reach the file the way `container.external_file[no] = ...` does. Images keep the existing video naming, `/_external_file_`. Co-authored-by: Claude Opus 5 --- dandi/cli/cmd_organize.py | 8 +- dandi/consts.py | 4 + dandi/files/__init__.py | 2 + dandi/files/_private.py | 7 +- dandi/files/bases.py | 6 ++ dandi/organize.py | 2 +- dandi/pynwb_utils.py | 156 +++++++++++++++++++++++++++++------ dandi/tests/fixtures.py | 81 ++++++++++++++++++ dandi/tests/test_files.py | 11 +++ dandi/tests/test_organize.py | 66 +++++++++++++++ 10 files changed, 312 insertions(+), 31 deletions(-) diff --git a/dandi/cli/cmd_organize.py b/dandi/cli/cmd_organize.py index 47930cbab..bcf3b106a 100644 --- a/dandi/cli/cmd_organize.py +++ b/dandi/cli/cmd_organize.py @@ -43,16 +43,16 @@ "--update-external-file-paths", is_flag=True, default=False, - help="Rewrite the 'external_file' arguments of ImageSeries in NWB files. " - "The new values will correspond to the new locations of the video files " - "after being organized. " + help="Rewrite the 'external_file' arguments of ImageSeries and the 'data' of " + "ExternalImage in NWB files. The new values will correspond to the new locations " + "of the referenced media files after being organized. " "This option requires --files-mode to be 'copy' or 'move'", ) @click.option( "--media-files-mode", type=EnumChoice(CopyMode), default=None, - help="How to relocate video files referenced by NWB files", + help="How to relocate media files referenced by NWB files", ) @click.option( "--required-field", diff --git a/dandi/consts.py b/dandi/consts.py index 145b39ee2..338c96713 100644 --- a/dandi/consts.py +++ b/dandi/consts.py @@ -190,6 +190,10 @@ def urls(self) -> Iterator[str]: VIDEO_FILE_EXTENSIONS = [".mp4", ".avi", ".wmv", ".mov", ".flv", ".mkv"] VIDEO_FILE_MODULES = ["processing", "acquisition"] +#: Extensions of the images an `ExternalImage` may point at. NWB restricts `image_format` to +#: PNG, JPEG and GIF, so nothing else can be referenced by an NWB file in the first place. +IMAGE_FILE_EXTENSIONS = [".png", ".jpg", ".jpeg", ".gif"] + ZARR_EXTENSIONS = [".ngff", ".zarr"] #: Maximum allowed depth of a Zarr directory tree diff --git a/dandi/files/__init__.py b/dandi/files/__init__.py index 9aee20847..8b1d501bc 100644 --- a/dandi/files/__init__.py +++ b/dandi/files/__init__.py @@ -29,6 +29,7 @@ DandiFile, DandisetMetadataFile, GenericAsset, + ImageAsset, LocalAsset, LocalDirectoryAsset, LocalFileAsset, @@ -51,6 +52,7 @@ "DandisetMetadataFile", "GenericAsset", "GenericBIDSAsset", + "ImageAsset", "LocalAsset", "LocalDirectoryAsset", "LocalFileAsset", diff --git a/dandi/files/_private.py b/dandi/files/_private.py index 4d8848059..ac6988feb 100644 --- a/dandi/files/_private.py +++ b/dandi/files/_private.py @@ -9,12 +9,13 @@ from dandi.consts import ( BIDS_DATASET_DESCRIPTION, + IMAGE_FILE_EXTENSIONS, VIDEO_FILE_EXTENSIONS, ZARR_EXTENSIONS, ) from dandi.exceptions import UnknownAssetError -from .bases import DandiFile, GenericAsset, LocalAsset, NWBAsset, VideoAsset +from .bases import DandiFile, GenericAsset, ImageAsset, LocalAsset, NWBAsset, VideoAsset from .bids import ( BIDSAsset, BIDSDatasetDescriptionAsset, @@ -33,6 +34,7 @@ class DandiFileType(Enum): VIDEO = 3 GENERIC = 4 BIDS_DATASET_DESCRIPTION = 5 + IMAGE = 6 @staticmethod def classify(path: Path) -> DandiFileType: @@ -50,6 +52,8 @@ def classify(path: Path) -> DandiFileType: return DandiFileType.NWB elif path.suffix in VIDEO_FILE_EXTENSIONS: return DandiFileType.VIDEO + elif path.suffix in IMAGE_FILE_EXTENSIONS: + return DandiFileType.IMAGE else: return DandiFileType.GENERIC @@ -61,6 +65,7 @@ class DandiFileFactory: DandiFileType.NWB: NWBAsset, DandiFileType.ZARR: ZarrAsset, DandiFileType.VIDEO: VideoAsset, + DandiFileType.IMAGE: ImageAsset, DandiFileType.GENERIC: GenericAsset, DandiFileType.BIDS_DATASET_DESCRIPTION: BIDSDatasetDescriptionAsset, } diff --git a/dandi/files/bases.py b/dandi/files/bases.py index 3577e605f..db502dcc3 100644 --- a/dandi/files/bases.py +++ b/dandi/files/bases.py @@ -628,6 +628,12 @@ class VideoAsset(LocalFileAsset): pass +class ImageAsset(LocalFileAsset): + """Representation of a local image file referenced by an NWB ``ExternalImage``""" + + pass + + class GenericAsset(LocalFileAsset): """ Representation of a generic regular file, one that is not of any known type diff --git a/dandi/organize.py b/dandi/organize.py index daf4fb9c2..c0ab2588a 100644 --- a/dandi/organize.py +++ b/dandi/organize.py @@ -961,7 +961,7 @@ def _get_metadata(path): ] raise ValueError( "--update-external-file-paths option not specified but found " - "external video files linked to the nwbfiles " + "external media files linked to the nwbfiles " f"{', '.join(files_list)}" ) diff --git a/dandi/pynwb_utils.py b/dandi/pynwb_utils.py index 75a4ee4a2..50806561f 100644 --- a/dandi/pynwb_utils.py +++ b/dandi/pynwb_utils.py @@ -34,6 +34,7 @@ from . import __version__, get_logger from .consts import ( + IMAGE_FILE_EXTENSIONS, VIDEO_FILE_EXTENSIONS, VIDEO_FILE_MODULES, metadata_nwb_computed_fields, @@ -349,7 +350,9 @@ def _get_session_duration(nwb: pynwb.NWBFile) -> float | None: st_data = obj["spike_times"].target if len(unit_end_idxs) > 1: - start = float(np.min(np.r_[st_data[0], st_data[unit_end_idxs[:-1]]])) + start = float( + np.min(np.r_[st_data[0], st_data[unit_end_idxs[:-1]]]) + ) else: start = float(st_data[0]) @@ -408,9 +411,16 @@ def _get_image_series(nwb: pynwb.NWBFile) -> list[dict]: module_cont = getattr(nwb, module_name) for name, ob in module_cont.items(): if isinstance(ob, pynwb.image.ImageSeries) and ob.external_file is not None: - out_dict = dict(id=ob.object_id, name=ob.name, external_files=[]) + out_dict = dict( + id=ob.object_id, + name=ob.name, + external_files=[], + field="external_file", + ) for ext_file in ob.external_file: - if (path := PurePosixPath(ext_file)).suffix in VIDEO_FILE_EXTENSIONS: + if ( + path := PurePosixPath(ext_file) + ).suffix in VIDEO_FILE_EXTENSIONS: out_dict["external_files"].append(path) else: lgr.warning( @@ -419,6 +429,48 @@ def _get_image_series(nwb: pynwb.NWBFile) -> list[dict]: ", ".join(VIDEO_FILE_EXTENSIONS), ) out.append(out_dict) + out.extend(_get_external_images(nwb)) + return out + + +def _get_external_images(nwb: pynwb.NWBFile) -> list[dict]: + """Retrieves all ExternalImage metadata from an open nwb file. + + An `ExternalImage` holds a single path or URL in ``data`` rather than a list in + ``external_file``, and it sits inside an `Images` container rather than directly in a module, + so the whole file is walked instead of the top level of the video modules. + + Parameters + ---------- + nwb: pynwb.NWBFile + + Returns + ------- + out: list[dict] + list of dicts : [{id: , name: , + external_files=[ExternalImage.data], field: "data"}] + """ + try: + from pynwb.base import ExternalImage + except ImportError: + # `ExternalImage` was added in pynwb 3.1.0; nothing to collect on an older one. + return [] + + out = [] + for ob in nwb.objects.values(): + if not isinstance(ob, ExternalImage) or ob.data is None: + continue + data = ob.data.decode() if isinstance(ob.data, bytes) else str(ob.data) + if (path := PurePosixPath(data)).suffix.lower() not in IMAGE_FILE_EXTENSIONS: + lgr.warning( + "external image %s should be one of: %s", + data, + ", ".join(IMAGE_FILE_EXTENSIONS), + ) + continue + out.append( + dict(id=ob.object_id, name=ob.name, external_files=[path], field="data") + ) return out @@ -443,28 +495,82 @@ def rename_nwb_external_files(metadata: list[dict], dandiset_path: str) -> None: ) return dandiset_nwbfile_path = op.join(dandiset_path, meta["dandi_path"]) - with NWBHDF5IO(dandiset_nwbfile_path, mode="r+", load_namespaces=True) as io: - nwb = io.read() - for ext_file_dict in meta["external_file_objects"]: - # retrieve nwb neurodata object of the given object id: - container_list = [ - child - for child in nwb.children - if ext_file_dict["id"] == child.object_id - ] - if len(container_list) == 0: - continue - else: - container = container_list[0] - # rename all external files: - for no, (name_old, name_new) in enumerate( - zip( - ext_file_dict["external_files"], - ext_file_dict["external_files_renamed"], - ) - ): - if not is_url(str(name_old)): - container.external_file[no] = str(name_new) + image_series = [ + d + for d in meta["external_file_objects"] + if d.get("field", "external_file") == "external_file" + ] + external_images = [ + d for d in meta["external_file_objects"] if d.get("field") == "data" + ] + if image_series: + with NWBHDF5IO( + dandiset_nwbfile_path, mode="r+", load_namespaces=True + ) as io: + nwb = io.read() + for ext_file_dict in image_series: + # retrieve nwb neurodata object of the given object id: + container_list = [ + child + for child in nwb.children + if ext_file_dict["id"] == child.object_id + ] + if len(container_list) == 0: + continue + else: + container = container_list[0] + # rename all external files: + for no, (name_old, name_new) in enumerate( + zip( + ext_file_dict["external_files"], + ext_file_dict["external_files_renamed"], + ) + ): + if not is_url(str(name_old)): + container.external_file[no] = str(name_new) + if external_images: + _rename_external_images(dandiset_nwbfile_path, external_images) + + +def _rename_external_images(nwbfile_path: str, external_images: list[dict]) -> None: + """Rewrites the ``data`` of the given `ExternalImage` objects in an NWB file on disk. + + `ExternalImage.data` is a scalar string rather than the array `ImageSeries.external_file` is, + so assigning through the read container does not reach the file and the dataset is written + directly instead. Objects are matched on the ``object_id`` attribute, since an `ExternalImage` + sits inside an `Images` container at a path this function is not told. + + Parameters + ---------- + nwbfile_path: str + full path of the NWB file to rewrite + external_images: list[dict] + the ``field == "data"`` entries of ``metadata["external_file_objects"]`` + """ + # Avoid a module level dependency on h5py for a path most callers never take: + import h5py + + renames = {} + for ext_file_dict in external_images: + for name_old, name_new in zip( + ext_file_dict["external_files"], ext_file_dict["external_files_renamed"] + ): + if not is_url(str(name_old)): + renames[ext_file_dict["id"]] = str(name_new) + if not renames: + return + + def rename_if_matched(_name: str, obj: Any) -> None: + if not isinstance(obj, h5py.Dataset): + return + object_id = obj.attrs.get("object_id") + if isinstance(object_id, bytes): + object_id = object_id.decode() + if object_id in renames: + obj[()] = renames[object_id] + + with h5py.File(nwbfile_path, "r+") as f: + f.visititems(rename_if_matched) @validate_cache.memoize_path diff --git a/dandi/tests/fixtures.py b/dandi/tests/fixtures.py index 4c4bb11a8..af6d567ac 100644 --- a/dandi/tests/fixtures.py +++ b/dandi/tests/fixtures.py @@ -19,6 +19,7 @@ import numpy as np import pynwb from pynwb import NWBHDF5IO, NWBFile +from pynwb.base import ExternalImage, Images from pynwb.device import Device from pynwb.file import Subject import pynwb.image @@ -849,6 +850,86 @@ def nwbfiles_video_common(video_files: list[tuple[Path, Path]]) -> Path: return _create_nwb_files(video_list) +#: A 1x1 red PNG. `organize` never decodes a referenced image, it only relocates the bytes, but +#: writing a real one keeps the fixture from resting on that. +MINIMAL_PNG = bytes.fromhex( + "89504e470d0a1a0a0000000d4948445200000001000000010802000000907753de" + "0000000c49444154789c63f8cfc0000003010100c9fe92ef0000000049454e44ae426082" +) + + +@pytest.fixture() +def image_files(tmp_path: Path) -> list[tuple[Path, Path]]: + image_paths = [] + image_path = tmp_path / "image_files" + image_path.mkdir() + for i in range(2): + image_file1 = image_path / f"test1_{i}.png" + image_file2 = image_path / f"test2_{i}.png" + image_file1.write_bytes(MINIMAL_PNG) + image_file2.write_bytes(MINIMAL_PNG) + image_paths.append((image_file1, image_file2)) + return image_paths + + +def _create_nwb_files_with_images(image_list: list[tuple[Path, Path]]) -> Path: + base_path = image_list[0][0].parent.parent + base_nwb_path = base_path / "nwbfiles" + base_nwb_path.mkdir(parents=True, exist_ok=True) + for i, image_loc in enumerate(image_list): + image_1 = image_loc[0] + image_2 = image_loc[1] + subject_id = f"mouse{i}" + session_id = f"sessionid{i}" + subject = Subject( + subject_id=subject_id, + species="Mus musculus", + sex="M", + description="lab mouse ", + ) + device = Device(name=f"imaging_device_{i}") + name = f"{image_1.stem}_{i}" + nwbfile = NWBFile( + f"{name}{i}", + "desc: contains images for dandi .png storage as external", + datetime.now(tzlocal()), + experimenter="Experimenter name", + session_id=session_id, + subject=subject, + devices=[device], + ) + + # An `ExternalImage` holds one path in `data`, so two of them stand where one `ImageSeries` + # holds a two-entry `external_file`. + images = Images( + name=f"MouseHistology{i}", + description="Images referenced by path rather than embedded", + images=[ + ExternalImage(name=f"slice{no}", data=str(image), image_format="PNG") + for no, image in enumerate([image_1, image_2]) + ], + ) + nwbfile.add_acquisition(images) + + nwbfile_path = base_nwb_path / f"{name}.nwb" + with NWBHDF5IO(str(nwbfile_path), "w") as io: + io.write(nwbfile) + return base_nwb_path + + +@pytest.fixture() +def nwbfiles_image_unique(image_files: list[tuple[Path, Path]]) -> Path: + """Create nwbfiles linked with unique set of images.""" + return _create_nwb_files_with_images(image_files) + + +@pytest.fixture() +def nwbfiles_image_common(image_files: list[tuple[Path, Path]]) -> Path: + """Create nwbfiles sharing image files.""" + image_list = [image_files[0], image_files[0]] + return _create_nwb_files_with_images(image_list) + + @pytest.fixture() def tmp_home( monkeypatch: pytest.MonkeyPatch, tmp_path_factory: pytest.TempPathFactory diff --git a/dandi/tests/test_files.py b/dandi/tests/test_files.py index adbc66f22..bc73aece2 100644 --- a/dandi/tests/test_files.py +++ b/dandi/tests/test_files.py @@ -22,6 +22,7 @@ DandisetMetadataFile, GenericAsset, GenericBIDSAsset, + ImageAsset, NWBAsset, NWBBIDSAsset, VideoAsset, @@ -60,6 +61,7 @@ def test_find_dandi_files(tmp_path: Path) -> None: "subdir/cleesh.txt", "empty.zarr/", "glarch.mp4", + "quux.png", ".ignored", ".ignored.dir/ignored.nwb", ) @@ -71,6 +73,9 @@ def test_find_dandi_files(tmp_path: Path) -> None: VideoAsset( filepath=tmp_path / "glarch.mp4", path="glarch.mp4", dandiset_path=tmp_path ), + ImageAsset( + filepath=tmp_path / "quux.png", path="quux.png", dandiset_path=tmp_path + ), ZarrAsset( filepath=tmp_path / "sample01.zarr", path="sample01.zarr", @@ -108,6 +113,9 @@ def test_find_dandi_files(tmp_path: Path) -> None: VideoAsset( filepath=tmp_path / "glarch.mp4", path="glarch.mp4", dandiset_path=tmp_path ), + ImageAsset( + filepath=tmp_path / "quux.png", path="quux.png", dandiset_path=tmp_path + ), ZarrAsset( filepath=tmp_path / "sample01.zarr", path="sample01.zarr", @@ -151,6 +159,9 @@ def test_find_dandi_files(tmp_path: Path) -> None: VideoAsset( filepath=tmp_path / "glarch.mp4", path="glarch.mp4", dandiset_path=tmp_path ), + ImageAsset( + filepath=tmp_path / "quux.png", path="quux.png", dandiset_path=tmp_path + ), ZarrAsset( filepath=tmp_path / "sample01.zarr", path="sample01.zarr", diff --git a/dandi/tests/test_organize.py b/dandi/tests/test_organize.py index c75d54ff1..50033b3a5 100644 --- a/dandi/tests/test_organize.py +++ b/dandi/tests/test_organize.py @@ -388,6 +388,72 @@ def test_video_organize_common( assert r.exit_code == rc +@mark_xfail_windows_python313_posixsubprocess +@pytest.mark.parametrize("mode", [FileOperationMode.COPY, FileOperationMode.MOVE]) +@pytest.mark.parametrize("image_mode", list(CopyMode)) +def test_image_organize( + image_mode: CopyMode, mode: FileOperationMode, nwbfiles_image_unique: Path +) -> None: + dandi_organize_path = nwbfiles_image_unique.parent / "dandi_organized" + cmd = [ + "--files-mode", + str(mode), + "--update-external-file-paths", + "--media-files-mode", + str(image_mode), + "-d", + str(dandi_organize_path), + str(nwbfiles_image_unique), + ] + image_files_list = list((nwbfiles_image_unique.parent / "image_files").iterdir()) + image_files_organized = [] + r = CliRunner().invoke(organize, cmd) + assert r.exit_code == 0 + for nwbfile_name in dandi_organize_path.glob("**/*.nwb"): + image_folder = nwbfile_name.with_suffix("") + assert image_folder.exists() + with NWBHDF5IO(str(nwbfile_name), "r", load_namespaces=True) as io: + nwbfile = io.read() + # get ExternalImage objects as dict(id=object_id, external_files=[]) + ext_file_objects = _get_image_series(nwbfile) + for ext_file_ob in ext_file_objects: + assert ext_file_ob["field"] == "data" + for no, name in enumerate(ext_file_ob["external_files"]): + image_files_organized.append(name) + # Regression: the rewritten paths are DANDI/S3 keys, + # must use forward slashes even on Windows. + assert "\\" not in str(name) + # check if the paths are correctly named according to convention: + filename = Path( + f"{image_folder.name}/{ext_file_ob['id']}_external_file_{no}" + ) + assert str(filename) == str(Path(name).with_suffix("")) + # check if the files exist( both in case of move/copy): + assert (image_folder.parent / name).exists() + # check all image files are organized: + assert len(image_files_list) == len(image_files_organized) + + +@mark_xfail_windows_python313_posixsubprocess +@pytest.mark.parametrize("image_mode,rc", [(CopyMode.COPY, 0), (CopyMode.MOVE, 1)]) +def test_image_organize_common( + image_mode: CopyMode, rc: int, nwbfiles_image_common: Path +) -> None: + dandi_organize_path = nwbfiles_image_common.parent / "dandi_organized" + cmd = [ + "--files-mode", + "move", + "--update-external-file-paths", + "--media-files-mode", + str(image_mode), + "-d", + str(dandi_organize_path), + str(nwbfiles_image_common), + ] + r = CliRunner().invoke(organize, cmd) + assert r.exit_code == rc + + @pytest.mark.parametrize( "path,error_ids", [ From f4fcb9910e5ad2c8e7855c8515e903bb367232ff Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Tue, 25 Aug 2026 21:57:24 -0600 Subject: [PATCH 2/4] Map IMAGE in the BIDS file factory as well `BIDSFileFactory` carries its own `CLASSES` table and a BIDS dataset can hold a `.jpg`, so classifying images without adding an entry there raised `KeyError: ` on every BIDS validation test. Images map to `GenericBIDSAsset`, the same as videos. Co-authored-by: Claude Opus 5 --- dandi/files/_private.py | 1 + 1 file changed, 1 insertion(+) diff --git a/dandi/files/_private.py b/dandi/files/_private.py index ac6988feb..38d93afda 100644 --- a/dandi/files/_private.py +++ b/dandi/files/_private.py @@ -88,6 +88,7 @@ class BIDSFileFactory(DandiFileFactory): DandiFileType.NWB: NWBBIDSAsset, DandiFileType.ZARR: ZarrBIDSAsset, DandiFileType.VIDEO: GenericBIDSAsset, + DandiFileType.IMAGE: GenericBIDSAsset, DandiFileType.GENERIC: GenericBIDSAsset, } From bbd85309bdd77c04153323c6ad5bd9eefb3bda1f Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Wed, 26 Aug 2026 09:03:03 -0600 Subject: [PATCH 3/4] Mark the image organize tests as ai_generated `DEVELOPMENT.md` requires the marker on any test written with AI assistance, and `test_image_organize` and `test_image_organize_common` were. Co-authored-by: Claude Opus 5 --- dandi/tests/test_organize.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dandi/tests/test_organize.py b/dandi/tests/test_organize.py index 50033b3a5..dbff65ff3 100644 --- a/dandi/tests/test_organize.py +++ b/dandi/tests/test_organize.py @@ -388,6 +388,7 @@ def test_video_organize_common( assert r.exit_code == rc +@pytest.mark.ai_generated @mark_xfail_windows_python313_posixsubprocess @pytest.mark.parametrize("mode", [FileOperationMode.COPY, FileOperationMode.MOVE]) @pytest.mark.parametrize("image_mode", list(CopyMode)) @@ -434,6 +435,7 @@ def test_image_organize( assert len(image_files_list) == len(image_files_organized) +@pytest.mark.ai_generated @mark_xfail_windows_python313_posixsubprocess @pytest.mark.parametrize("image_mode,rc", [(CopyMode.COPY, 0), (CopyMode.MOVE, 1)]) def test_image_organize_common( From 546549216a11725655243bda5b6b7af12d289ad8 Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Wed, 26 Aug 2026 14:43:27 -0600 Subject: [PATCH 4/4] Empty commit to re-trigger CI