Merge pull request #594 from catalystneuro/change_imaging_plane_ident…

…ifier_to_string

Change `add_imaging_plane` to add imaging plane by name instead of index

CHANGELOG.md

@@ -2,7 +2,8 @@
 
 ### Features
 Added `session_start_time` extraction to `FicTracDataInterface`. [PR #598](https://github.com/catalystneuro/neuroconv/pull/598)
-
+* Added `imaging_plane_name` keyword argument to `add_imaging_plane` function to determine which imaging plane to add from the metadata by name instead of `imaging_plane_index`.
+  Added reference for `imaging_plane` to default plane segmentation metadata. [PR #594](https://github.com/catalystneuro/neuroconv/pull/594)
 
 ### Fixes
 Remove `starting_time` reset to default value (0.0) when adding the rate and updating the `photon_series_kwargs` or `roi_response_series_kwargs`, in `add_photon_series` or `add_fluorescence_traces`. [PR #595](https://github.com/catalystneuro/neuroconv/pull/595)
@@ -16,13 +17,11 @@ Remove `starting_time` reset to default value (0.0) when adding the rate and upd
 * Changed the date parsing in `OpenEphysLegacyRecordingInterface` to `datetime.strptime` with the expected date format explicitly set to `"%d-%b-%Y %H%M%S"`. [PR #577](https://github.com/catalystneuro/neuroconv/pull/577)
 * Pin lower bound HDMF version to `3.10.0`. [PR #586](https://github.com/catalystneuro/neuroconv/pull/586)
 
-
 ### Deprecation
 * Removed `use_times` and `buffer_size` from `add_photon_series`. [PR #600](https://github.com/catalystneuro/neuroconv/pull/600)
 
 
 
-
 # v0.4.4
 
 ### Features

src/neuroconv/tools/roiextractors/roiextractors.py

@@ -103,6 +103,7 @@ def get_default_segmentation_metadata() -> DeepDict:
             dict(
                 name="PlaneSegmentation",
                 description="Segmented ROIs",
+                imaging_plane=metadata["Ophys"]["ImagingPlane"][0]["name"],
             )
         ],
     )
@@ -216,7 +217,12 @@ def _create_imaging_plane_from_metadata(nwbfile: NWBFile, imaging_plane_metadata
     return imaging_plane
 
 
-def add_imaging_plane(nwbfile: NWBFile, metadata: dict, imaging_plane_index: int = 0) -> NWBFile:
+def add_imaging_plane(
+    nwbfile: NWBFile,
+    metadata: dict,
+    imaging_plane_name: Optional[str] = None,
+    imaging_plane_index: Optional[int] = None,
+) -> NWBFile:
     """
     Adds the imaging plane specified by the metadata to the nwb file.
     The imaging plane that is added is the one located in metadata["Ophys"]["ImagingPlane"][imaging_plane_index]
@@ -227,27 +233,45 @@ def add_imaging_plane(nwbfile: NWBFile, metadata: dict, imaging_plane_index: int
         An previously defined -in memory- NWBFile.
     metadata : dict
         The metadata in the nwb conversion tools format.
-    imaging_plane_index : int, default: 0
-        The metadata in the nwb conversion tools format is a list of the different imaging planes to add.
-        Specify which element of the list with this parameter.
-
+    imaging_plane_name: str
+        The name of the imaging plane to be added.
     Returns
     -------
     NWBFile
         The nwbfile passed as an input with the imaging plane added.
     """
+    if imaging_plane_index is not None:
+        warn(
+            message="Keyword argument 'imaging_plane_index' is deprecated and will be removed on or after Dec 1st, 2023. "
+            "Use 'imaging_plane_name' to specify which imaging plane to add by its name.",
+            category=DeprecationWarning,
+        )
+        imaging_plane_name = metadata["Ophys"]["ImagingPlane"][imaging_plane_index]["name"]
 
     # Set the defaults and required infrastructure
     metadata_copy = deepcopy(metadata)
     default_metadata = get_default_ophys_metadata()
     metadata_copy = dict_deep_update(default_metadata, metadata_copy, append_list=False)
     add_devices(nwbfile=nwbfile, metadata=metadata_copy)
 
-    imaging_plane_metadata = metadata_copy["Ophys"]["ImagingPlane"][imaging_plane_index]
-    imaging_plane_name = imaging_plane_metadata["name"]
+    default_imaging_plane_name = default_metadata["Ophys"]["ImagingPlane"][0]["name"]
+    imaging_plane_name = imaging_plane_name or default_imaging_plane_name
     existing_imaging_planes = nwbfile.imaging_planes
 
     if imaging_plane_name not in existing_imaging_planes:
+        imaging_plane_metadata = next(
+            (
+                imaging_plane_metadata
+                for imaging_plane_metadata in metadata_copy["Ophys"]["ImagingPlane"]
+                if imaging_plane_metadata["name"] == imaging_plane_name
+            ),
+            None,
+        )
+        if imaging_plane_metadata is None:
+            raise ValueError(
+                f"Metadata for Imaging Plane '{imaging_plane_name}' not found in metadata['Ophys']['ImagingPlane']."
+            )
+
         imaging_plane = _create_imaging_plane_from_metadata(
             nwbfile=nwbfile, imaging_plane_metadata=imaging_plane_metadata
         )
@@ -346,18 +370,18 @@ def add_photon_series(
         ), "Received metadata for 'OnePhotonSeries' but `photon_series_type` was not explicitly specified."
 
     # Tests if TwoPhotonSeries//OnePhotonSeries already exists in acquisition
-    photon_series_kwargs = metadata_copy["Ophys"][photon_series_type][photon_series_index]
-    photon_series_name = photon_series_kwargs["name"]
+    photon_series_metadata = metadata_copy["Ophys"][photon_series_type][photon_series_index]
+    photon_series_name = photon_series_metadata["name"]
 
     if photon_series_name in nwbfile.acquisition:
         warn(f"{photon_series_name} already on nwbfile")
         return nwbfile
 
     # Add the image plane to nwb
-    # TODO: change imaging_plane_index to photon_series_key
-    nwbfile = add_imaging_plane(nwbfile=nwbfile, metadata=metadata_copy, imaging_plane_index=photon_series_index)
-    imaging_plane_name = photon_series_kwargs["imaging_plane"]
+    imaging_plane_name = photon_series_metadata["imaging_plane"]
+    add_imaging_plane(nwbfile=nwbfile, metadata=metadata_copy, imaging_plane_name=imaging_plane_name)
     imaging_plane = nwbfile.get_imaging_plane(name=imaging_plane_name)
+    photon_series_kwargs = deepcopy(photon_series_metadata)
     photon_series_kwargs.update(imaging_plane=imaging_plane)
 
     # Add the data
@@ -681,7 +705,8 @@ def add_plane_segmentation(
     plane_segmentation_metadata = image_segmentation_metadata["plane_segmentations"][plane_segmentation_index]
     plane_segmentation_name = plane_segmentation_metadata["name"]
 
-    add_imaging_plane(nwbfile=nwbfile, metadata=metadata_copy, imaging_plane_index=plane_segmentation_index)
+    imaging_plane_name = plane_segmentation_metadata["imaging_plane"]
+    add_imaging_plane(nwbfile=nwbfile, metadata=metadata_copy, imaging_plane_name=imaging_plane_name)
     add_image_segmentation(nwbfile=nwbfile, metadata=metadata_copy)
 
     ophys = get_module(nwbfile, "ophys")
@@ -700,7 +725,8 @@ def add_plane_segmentation(
         imaging_plane_name = imaging_plane_metadata["name"]
         imaging_plane = nwbfile.imaging_planes[imaging_plane_name]
 
-        plane_segmentation_kwargs = dict(**plane_segmentation_metadata, imaging_plane=imaging_plane)
+        plane_segmentation_kwargs = deepcopy(plane_segmentation_metadata)
+        plane_segmentation_kwargs.update(imaging_plane=imaging_plane)
         if mask_type is None:
             plane_segmentation = PlaneSegmentation(id=roi_ids, **plane_segmentation_kwargs)
         elif mask_type == "image":

tests/test_ophys/test_tools_roiextractors.py

@@ -164,7 +164,7 @@ def test_device_object_and_metadata_mix(self):
         assert "device_object" in devices
 
 
-class TestAddImagingPlane(unittest.TestCase):
+class TestAddImagingPlane(TestCase):
     def setUp(self):
         self.session_start_time = datetime.now().astimezone()
         self.nwbfile = NWBFile(
@@ -200,7 +200,7 @@ def setUp(self):
         self.metadata["Ophys"].update(ImagingPlane=[self.imaging_plane_metadata])
 
     def test_add_imaging_plane(self):
-        add_imaging_plane(nwbfile=self.nwbfile, metadata=self.metadata)
+        add_imaging_plane(nwbfile=self.nwbfile, metadata=self.metadata, imaging_plane_name=self.imaging_plane_name)
 
         imaging_planes = self.nwbfile.imaging_planes
         assert len(imaging_planes) == 1
@@ -210,10 +210,10 @@ def test_add_imaging_plane(self):
         assert imaging_plane.description == self.imaging_plane_description
 
     def test_not_overwriting_imaging_plane_if_same_name(self):
-        add_imaging_plane(nwbfile=self.nwbfile, metadata=self.metadata)
+        add_imaging_plane(nwbfile=self.nwbfile, metadata=self.metadata, imaging_plane_name=self.imaging_plane_name)
 
         self.imaging_plane_metadata["description"] = "modified description"
-        add_imaging_plane(nwbfile=self.nwbfile, metadata=self.metadata)
+        add_imaging_plane(nwbfile=self.nwbfile, metadata=self.metadata, imaging_plane_name=self.imaging_plane_name)
 
         imaging_planes = self.nwbfile.imaging_planes
         assert len(imaging_planes) == 1
@@ -225,14 +225,14 @@ def test_add_two_imaging_planes(self):
         first_imaging_plane_description = "first_imaging_plane_description"
         self.imaging_plane_metadata["name"] = first_imaging_plane_name
         self.imaging_plane_metadata["description"] = first_imaging_plane_description
-        add_imaging_plane(nwbfile=self.nwbfile, metadata=self.metadata)
+        add_imaging_plane(nwbfile=self.nwbfile, metadata=self.metadata, imaging_plane_name=first_imaging_plane_name)
 
         # Add the second imaging plane
         second_imaging_plane_name = "second_imaging_plane_name"
         second_imaging_plane_description = "second_imaging_plane_description"
         self.imaging_plane_metadata["name"] = second_imaging_plane_name
         self.imaging_plane_metadata["description"] = second_imaging_plane_description
-        add_imaging_plane(nwbfile=self.nwbfile, metadata=self.metadata)
+        add_imaging_plane(nwbfile=self.nwbfile, metadata=self.metadata, imaging_plane_name=second_imaging_plane_name)
 
         # Test expected values
         imaging_planes = self.nwbfile.imaging_planes
@@ -246,6 +246,49 @@ def test_add_two_imaging_planes(self):
         assert second_imaging_plane.name == second_imaging_plane_name
         assert second_imaging_plane.description == second_imaging_plane_description
 
+    def test_add_imaging_plane_raises_when_name_not_found_in_metadata(self):
+        """Test adding an imaging plane raises an error when the name is not found in the metadata."""
+        imaging_plane_name = "imaging_plane_non_existing_in_the_metadata"
+        with self.assertRaisesWith(
+            exc_type=ValueError,
+            exc_msg=f"Metadata for Imaging Plane '{imaging_plane_name}' not found in metadata['Ophys']['ImagingPlane'].",
+        ):
+            add_imaging_plane(nwbfile=self.nwbfile, metadata=self.metadata, imaging_plane_name=imaging_plane_name)
+
+    def test_add_two_imaging_planes_from_metadata(self):
+        """Test adding two imaging planes when there are multiple imaging plane metadata."""
+
+        second_imaging_plane_name = "second_imaging_plane_name"
+        metadata = deepcopy(self.metadata)
+        imaging_planes_metadata = metadata["Ophys"]["ImagingPlane"]
+        second_imaging_plane_metadata = deepcopy(metadata["Ophys"]["ImagingPlane"][0])
+        second_imaging_plane_metadata.update(name="second_imaging_plane_name")
+        imaging_planes_metadata.append(second_imaging_plane_metadata)
+        add_imaging_plane(nwbfile=self.nwbfile, metadata=metadata, imaging_plane_name=self.imaging_plane_name)
+        add_imaging_plane(nwbfile=self.nwbfile, metadata=metadata, imaging_plane_name="second_imaging_plane_name")
+
+        # Test expected values
+        imaging_planes = self.nwbfile.imaging_planes
+        assert len(imaging_planes) == 2
+
+        first_imaging_plane = imaging_planes[self.imaging_plane_name]
+        assert first_imaging_plane.name == self.imaging_plane_name
+
+        second_imaging_plane = imaging_planes[second_imaging_plane_name]
+        assert second_imaging_plane.name == second_imaging_plane_name
+
+    def test_add_imaging_plane_warns_when_index_is_used(self):
+        """Test adding an imaging plane with the index specified warns with DeprecationWarning."""
+        exc_msg = "Keyword argument 'imaging_plane_index' is deprecated and will be removed on or after Dec 1st, 2023. Use 'imaging_plane_name' to specify which imaging plane to add by its name."
+        with self.assertWarnsWith(warn_type=DeprecationWarning, exc_msg=exc_msg):
+            add_imaging_plane(nwbfile=self.nwbfile, metadata=self.metadata, imaging_plane_index=0)
+            # Test expected values
+            imaging_planes = self.nwbfile.imaging_planes
+            assert len(imaging_planes) == 1
+
+            imaging_plane = imaging_planes[self.imaging_plane_name]
+            assert imaging_plane.name == self.imaging_plane_name
+
 
 class TestAddImageSegmentation(unittest.TestCase):
     def setUp(self):
@@ -331,8 +374,7 @@ def setUp(self):
         self.metadata = dict(Ophys=dict())
 
         self.plane_segmentation_metadata = dict(
-            name=self.plane_segmentation_name,
-            description="Segmented ROIs",
+            name=self.plane_segmentation_name, description="Segmented ROIs", imaging_plane="ImagingPlane"
         )
 
         image_segmentation_metadata = dict(
@@ -1427,6 +1469,33 @@ def test_add_one_photon_series_roundtrip(self):
             expected_one_photon_series_shape = (self.num_frames, self.num_columns, self.num_rows)
             assert one_photon_series.shape == expected_one_photon_series_shape
 
+    def test_add_multiple_one_photon_series_with_same_imaging_plane(self):
+        """Test adding two OnePhotonSeries that use the same ImagingPlane."""
+        shared_photon_series_metadata = deepcopy(self.one_photon_series_metadata)
+        shared_imaging_plane_name = "same_imaging_plane_for_two_series"
+
+        shared_photon_series_metadata["Ophys"]["ImagingPlane"][0]["name"] = shared_imaging_plane_name
+        shared_photon_series_metadata["Ophys"]["OnePhotonSeries"][0]["imaging_plane"] = shared_imaging_plane_name
+
+        add_photon_series(
+            imaging=self.imaging_extractor,
+            nwbfile=self.nwbfile,
+            metadata=shared_photon_series_metadata,
+            photon_series_type="OnePhotonSeries",
+        )
+
+        shared_photon_series_metadata["Ophys"]["OnePhotonSeries"][0]["name"] = "second_photon_series"
+        add_photon_series(
+            imaging=self.imaging_extractor,
+            nwbfile=self.nwbfile,
+            metadata=shared_photon_series_metadata,
+            photon_series_type="OnePhotonSeries",
+        )
+
+        self.assertIn("second_photon_series", self.nwbfile.acquisition)
+        self.assertEqual(len(self.nwbfile.imaging_planes), 1)
+        self.assertIn(shared_imaging_plane_name, self.nwbfile.imaging_planes)
+
 
 class TestAddSummaryImages(unittest.TestCase):
     def setUp(self):