From 6f40e4e90c60dedfec134cf40370c20274f33884 Mon Sep 17 00:00:00 2001 From: Doug Ollerenshaw Date: Mon, 12 Oct 2026 17:51:26 -0700 Subject: [PATCH 1/4] First pass at updating mapper to accomodate new fip.json --- src/aind_metadata_mapper/fip/README.md | 6 +- src/aind_metadata_mapper/fip/mapper.py | 132 ++++++++++++++++--------- tests/fip/test_mapper.py | 76 ++++++++++++++ tests/fixtures/fip_intermediate.json | 78 ++++++++++++++- 4 files changed, 245 insertions(+), 47 deletions(-) diff --git a/src/aind_metadata_mapper/fip/README.md b/src/aind_metadata_mapper/fip/README.md index 46e4cec9..dd416c6d 100644 --- a/src/aind_metadata_mapper/fip/README.md +++ b/src/aind_metadata_mapper/fip/README.md @@ -51,7 +51,7 @@ acquisition.write_standard_file(output_directory=Path("/output/path"), suffix=No The mapper expects input JSON with this structure: - `session` - experiment metadata (subject, experimenters, notes, etc.) - `rig` - hardware configuration (cameras, LEDs, ROI settings, etc.) -- `data_stream_metadata` - timing information +- `data_stream_metadata` - timing information and the ROI settings (`regions`) used in each stream ## What Gets Mapped @@ -69,7 +69,9 @@ The mapper expects input JSON with this structure: - `rig.camera_green_iso` → Green/Isosbestic detector - `rig.camera_red` → Red detector - `rig.cuttlefish_fip` → Controller device -- `rig.roi_settings` → Determines fiber count and creates patch cord configurations +- `data_stream_metadata[*].regions` → Camera identifiers for patch cord configurations. Streams without `regions` (data from aind-physiology-fip before 0.3.5) are filled from `rig.roi_settings` before validation, with a warning; if that is also null the mapper raises `ValueError`. + +The mapper assumes each ROI is drawn over its index-matched patch cord (ROI 0 over Patch Cord 0, and so on). Patch cords are created for each implanted fiber from the procedures record; the ROI settings only identify which cameras had ROIs drawn, and regions from all streams are merged by key. ### Channels Each fiber gets three channels: diff --git a/src/aind_metadata_mapper/fip/mapper.py b/src/aind_metadata_mapper/fip/mapper.py index b5083943..17863098 100644 --- a/src/aind_metadata_mapper/fip/mapper.py +++ b/src/aind_metadata_mapper/fip/mapper.py @@ -345,6 +345,10 @@ def transform( If metadata validation fails, or if a provided ethics_review_id does not match the IACUC protocol on record in LabTracks for the subject. """ + # Fill in per-stream `regions` from `rig.roi_settings` for data recorded before + # aind-physiology-fip 0.3.5, so one shape flows through validation and mapping (#498) + metadata = self._normalize_legacy_roi_settings(metadata) + # Validate metadata against JSON schema unless skipped if not skip_validation: self._validate_fip_metadata(metadata) @@ -357,6 +361,9 @@ def transform( subject_id = session["subject"] instrument_id = rig["rig_name"] + # Merge each stream's ROI settings by key; only the keys (which cameras had ROIs) are used + roi_settings = {k: v for ds in data_streams for k, v in ds["regions"].items()} + ethics_review_id = self._resolve_ethics_review_id(subject_id, ethics_review_id) # Get timing from all data streams (handle multiple epochs) @@ -423,7 +430,7 @@ def transform( modalities=[Modality.FIB], code=code, active_devices=self._get_active_devices(rig, implanted_fibers), - configurations=self._build_configurations(rig, implanted_fibers, intended_measurements), + configurations=self._build_configurations(rig, implanted_fibers, roi_settings, intended_measurements), connections=self._build_connections(implanted_fibers), ) @@ -495,6 +502,42 @@ def _extract_camera_exposure_time(self, rig_config: Dict) -> float: ) return 10000.0 # Default exposure time in microseconds + def _normalize_legacy_roi_settings(self, metadata: dict) -> dict: + """Fill in per-stream ``regions`` from ``rig.roi_settings`` for legacy input. + + aind-physiology-fip 0.3.5 and later record the ROI settings used by each data stream + under ``regions``. Older data only has ``rig.roi_settings``, the definition loaded + before the experiment, so it is copied into any stream lacking ``regions`` (with a + warning). Streams that already have ``regions`` are left as is. The input is not + modified; metadata without data streams is returned unchanged for schema validation + to report. + + Raises + ------ + ValueError + If a stream has no ``regions`` and ``rig.roi_settings`` is also missing or null. + aind-data-schema requires a PatchCordConfig for the fiber photometry modality, and + those are built from the ROI settings, so the acquisition cannot be created. + """ + rig_roi_settings = (metadata.get("rig") or {}).get("roi_settings") + streams = [] + for stream in metadata.get("data_stream_metadata") or []: + if not stream.get("regions"): + if not rig_roi_settings: + raise ValueError( + f"Data stream '{stream.get('id')}' has no regions and rig.roi_settings is missing or " + "null in the extracted FIP metadata. Patch cord configurations are built from the ROI " + "settings, and aind-data-schema requires a PatchCordConfig for the fiber photometry " + "modality, so the acquisition cannot be created without them." + ) + logger.warning( + f"Data stream '{stream.get('id')}' has no regions; falling back to rig.roi_settings. " + "Per-stream ROI settings are recorded by aind-physiology-fip >= 0.3.5." + ) + stream = {**stream, "regions": rig_roi_settings} + streams.append(stream) + return {**metadata, "data_stream_metadata": streams} if streams else metadata + def _get_camera_names_from_roi(self, roi_settings: Dict) -> Dict[str, str]: """Get camera device identifiers from ROI settings. @@ -625,6 +668,7 @@ def _build_configurations( self, rig_config: Dict[str, Any], implanted_fibers: List[int], + roi_settings: Dict[str, Any], intended_measurements: Optional[Dict[str, Dict[str, Optional[str]]]] = None, ) -> List[Any]: """Build device configurations from rig config. @@ -633,6 +677,8 @@ def _build_configurations( ---------- rig_config : Dict[str, Any] Rig configuration dictionary from metadata. + roi_settings : Dict[str, Any] + ROI settings merged across data streams; identifies which cameras had ROIs drawn. intended_measurements : Optional[Dict[str, Dict[str, Optional[str]]]], optional Intended measurements from metadata service, mapping fiber names to channel measurements (R, G, B, Iso), by default None. @@ -664,51 +710,49 @@ def _build_configurations( # 1. Green: 470nm excitation, ~520nm emission, green camera # 2. Isosbestic: 415nm excitation, ~520nm emission, green camera (same detector!) # 3. Red: 565nm excitation, ~590nm emission, red camera - roi_settings = rig_config.get("roi_settings", {}) - if roi_settings: - # Get camera identifiers from ROI settings - camera_names = self._get_camera_names_from_roi(roi_settings) - green_camera_name = camera_names.get("green") - red_camera_name = camera_names.get("red") - - # Use implanted fibers directly - fiber_indices = implanted_fibers - - # Create patch cord for each implanted fiber - for fiber_idx in fiber_indices: - channels = [] - fiber_name = f"Fiber_{fiber_idx}" - fiber_measurements = intended_measurements.get(fiber_name) if intended_measurements else None - - # Channel definitions: (channel_type, measurement_key, led_wavelength, camera_name, emission_wavelength) - channel_defs = [ - ("Green", "G", EXCITATION_BLUE, green_camera_name, EMISSION_GREEN), - ("Isosbestic", "Iso", EXCITATION_UV, green_camera_name, EMISSION_GREEN), - ("Red", "R", EXCITATION_YELLOW, red_camera_name, EMISSION_RED), - ] - - for channel_type, measurement_key, led_wavelength, camera_name, emission_wavelength in channel_defs: - if camera_name: - measurement = fiber_measurements.get(measurement_key) if fiber_measurements else None - channels.append( - self._create_channel( - fiber_idx, - channel_type, - led_configs_by_wavelength.get(led_wavelength), - measurement, - camera_name, - emission_wavelength, - exposure_time_ms, - ) - ) + # Get camera identifiers from ROI settings + camera_names = self._get_camera_names_from_roi(roi_settings) + green_camera_name = camera_names.get("green") + red_camera_name = camera_names.get("red") + + # Use implanted fibers directly + fiber_indices = implanted_fibers + + # Create patch cord for each implanted fiber + for fiber_idx in fiber_indices: + channels = [] + fiber_name = f"Fiber_{fiber_idx}" + fiber_measurements = intended_measurements.get(fiber_name) if intended_measurements else None + + # Channel definitions: (channel_type, measurement_key, led_wavelength, camera_name, emission_wavelength) + channel_defs = [ + ("Green", "G", EXCITATION_BLUE, green_camera_name, EMISSION_GREEN), + ("Isosbestic", "Iso", EXCITATION_UV, green_camera_name, EMISSION_GREEN), + ("Red", "R", EXCITATION_YELLOW, red_camera_name, EMISSION_RED), + ] - # Create patch cord if we have channels - if channels: - patch_cord = PatchCordConfig( - device_name=f"Patch Cord {fiber_idx}", - channels=channels, + for channel_type, measurement_key, led_wavelength, camera_name, emission_wavelength in channel_defs: + if camera_name: + measurement = fiber_measurements.get(measurement_key) if fiber_measurements else None + channels.append( + self._create_channel( + fiber_idx, + channel_type, + led_configs_by_wavelength.get(led_wavelength), + measurement, + camera_name, + emission_wavelength, + exposure_time_ms, + ) ) - configurations.append(patch_cord) + + # Create patch cord if we have channels + if channels: + patch_cord = PatchCordConfig( + device_name=f"Patch Cord {fiber_idx}", + channels=channels, + ) + configurations.append(patch_cord) return configurations diff --git a/tests/fip/test_mapper.py b/tests/fip/test_mapper.py index 2e47701e..2c83f72a 100644 --- a/tests/fip/test_mapper.py +++ b/tests/fip/test_mapper.py @@ -42,6 +42,7 @@ def setUp(self): } self.test_implanted_fibers = [0, 1] # Two implanted fibers self.test_ethics_review_id = ["2414"] # Injected to keep tests network-free + self.test_regions = self.example_intermediate_data["data_stream_metadata"][0]["regions"] # transform always calls get_iacuc_protocol to verify ethics_review_id against # LabTracks, so mock it here to keep all tests network-free. Individual tests @@ -50,6 +51,16 @@ def setUp(self): self.mock_get_iacuc_protocol = patcher.start() self.addCleanup(patcher.stop) + def _transform(self, data, **overrides): + """Run transform on ``data`` with network-free test inputs, overridable per call.""" + kwargs = dict( + skip_validation=True, + intended_measurements=self.test_intended_measurements, + implanted_fibers=self.test_implanted_fibers, + ethics_review_id=self.test_ethics_review_id, + ) + return self.mapper.transform(data, **{**kwargs, **overrides}) + def test_mapper_initialization(self): """Test that FIPMapper can be instantiated with default configuration. @@ -206,6 +217,7 @@ def test_code_field_from_rig_version(self): "id": "test_stream", "start_time": "2025-07-18 19:32:35.275046+00:00", "end_time": "2025-07-18 19:49:22.448358+00:00", + "regions": self.test_regions, } ], "session": { @@ -451,16 +463,19 @@ def test_multiple_epochs_uses_earliest_and_latest_times(self): "id": "fip_epoch_0", "start_time": t0.isoformat(), "end_time": t1.isoformat(), + "regions": self.test_regions, }, { "id": "fip_epoch_1", "start_time": t2.isoformat(), "end_time": t3.isoformat(), + "regions": self.test_regions, }, { "id": "fip_epoch_2", "start_time": t4.isoformat(), "end_time": t5.isoformat(), + "regions": self.test_regions, }, ], "session": { @@ -590,6 +605,67 @@ def test_transform_missing_implanted_fibers_in_procedures(self): # Restore original method self.mapper._parse_implanted_fibers = original_method + def test_transform_fails_when_no_roi_settings(self): + """Test the clear ValueError when a stream has no regions and rig.roi_settings is also missing (#498).""" + stream = self.example_intermediate_data["data_stream_metadata"][0] + legacy_stream = {k: v for k, v in stream.items() if k != "regions"} + rig = {k: v for k, v in self.example_intermediate_data["rig"].items() if k != "roi_settings"} + cases = ( + ("null", {**legacy_stream, "regions": None}, {**rig, "roi_settings": None}), + ("absent", legacy_stream, rig), + ) + for label, bad_stream, bad_rig in cases: + with self.subTest(fields=label): + data = {**self.example_intermediate_data, "data_stream_metadata": [bad_stream], "rig": bad_rig} + with self.assertRaises(ValueError) as cm: + self._transform(data) + self.assertIn(stream["id"], str(cm.exception)) + self.assertIn("rig.roi_settings", str(cm.exception)) + + def test_rig_roi_settings_ignored_when_regions_present(self): + """Test that rig.roi_settings has no effect when every stream has regions (they take precedence).""" + baseline = self._transform(self.example_intermediate_data) + rig = {k: v for k, v in self.example_intermediate_data["rig"].items() if k != "roi_settings"} + for label, bad_rig in (("null", {**rig, "roi_settings": None}), ("absent", rig)): + with self.subTest(roi_settings=label): + self.assertEqual(self._transform({**self.example_intermediate_data, "rig": bad_rig}), baseline) + + def test_legacy_streams_fall_back_to_rig_roi_settings(self): + """Test that streams without regions are filled from rig.roi_settings before validation. + + Legacy data (aind-physiology-fip before 0.3.5) must validate, map identically to the + new shape, log a warning, and leave the caller's input untouched. + """ + baseline = self._transform(self.example_intermediate_data) + stream = self.example_intermediate_data["data_stream_metadata"][0] + legacy_stream = {k: v for k, v in stream.items() if k != "regions"} + for label, old_stream in (("null", {**legacy_stream, "regions": None}), ("absent", legacy_stream)): + with self.subTest(regions=label): + data = {**self.example_intermediate_data, "data_stream_metadata": [old_stream]} + with self.assertLogs(mapper_mod.logger, level="WARNING") as logs: + acquisition = self._transform(data, skip_validation=False) + self.assertEqual(acquisition, baseline) + self.assertIn("falling back to rig.roi_settings", logs.output[0]) + self.assertNotIn("regions", legacy_stream) + + def test_regions_merged_across_streams(self): + """Test that camera identifiers are collected from the regions of every data stream.""" + stream = self.example_intermediate_data["data_stream_metadata"][0] + green_only = {k: v for k, v in self.test_regions.items() if k != "camera_red_roi"} + red_only = {k: v for k, v in self.test_regions.items() if k != "camera_green_iso_roi"} + + def detectors(streams): + """Return the detector names across all patch cord channels for the given streams.""" + configs = self._transform({**self.example_intermediate_data, "data_stream_metadata": streams}) + patch_cords = [c for c in configs.data_streams[0].configurations if type(c).__name__ == "PatchCordConfig"] + return {ch.detector.device_name for pc in patch_cords for ch in pc.channels} + + self.assertEqual(detectors([{**stream, "regions": green_only}]), {"Green CMOS"}) + self.assertEqual( + detectors([{**stream, "id": "a", "regions": green_only}, {**stream, "id": "b", "regions": red_only}]), + {"Green CMOS", "Red CMOS"}, + ) + def test_validate_fip_metadata_error(self): """Test that _validate_fip_metadata raises ValueError on validation failure. diff --git a/tests/fixtures/fip_intermediate.json b/tests/fixtures/fip_intermediate.json index e4432f22..ef7f832b 100644 --- a/tests/fixtures/fip_intermediate.json +++ b/tests/fixtures/fip_intermediate.json @@ -3,7 +3,83 @@ { "id": "fip_2025-07-18T192959", "start_time": "2025-07-18 12:32:35.275046-07:00", - "end_time": "2025-07-18 12:49:22.448358-07:00" + "end_time": "2025-07-18 12:49:22.448358-07:00", + "regions": { + "camera_green_iso_background": { + "center": { + "x": 10.0, + "y": 10.0 + }, + "radius": 10.0 + }, + "camera_red_background": { + "center": { + "x": 10.0, + "y": 10.0 + }, + "radius": 10.0 + }, + "camera_green_iso_roi": [ + { + "center": { + "x": 50.0, + "y": 50.0 + }, + "radius": 20.0 + }, + { + "center": { + "x": 50.0, + "y": 150.0 + }, + "radius": 20.0 + }, + { + "center": { + "x": 150.0, + "y": 50.0 + }, + "radius": 20.0 + }, + { + "center": { + "x": 150.0, + "y": 150.0 + }, + "radius": 20.0 + } + ], + "camera_red_roi": [ + { + "center": { + "x": 50.0, + "y": 50.0 + }, + "radius": 20.0 + }, + { + "center": { + "x": 50.0, + "y": 150.0 + }, + "radius": 20.0 + }, + { + "center": { + "x": 150.0, + "y": 50.0 + }, + "radius": 20.0 + }, + { + "center": { + "x": 150.0, + "y": 150.0 + }, + "radius": 20.0 + } + ] + } } ], "session": { From 8929a3729ca0ac45d08c54815cac4db3bce6091e Mon Sep 17 00:00:00 2001 From: Doug Ollerenshaw Date: Mon, 12 Oct 2026 18:03:27 -0700 Subject: [PATCH 2/4] Remove the function that normalizes old format data --- src/aind_metadata_mapper/fip/README.md | 2 +- src/aind_metadata_mapper/fip/mapper.py | 38 +++++++++++--------------- tests/fip/test_mapper.py | 5 ++-- 3 files changed, 19 insertions(+), 26 deletions(-) diff --git a/src/aind_metadata_mapper/fip/README.md b/src/aind_metadata_mapper/fip/README.md index dd416c6d..2b1e20a6 100644 --- a/src/aind_metadata_mapper/fip/README.md +++ b/src/aind_metadata_mapper/fip/README.md @@ -69,7 +69,7 @@ The mapper expects input JSON with this structure: - `rig.camera_green_iso` → Green/Isosbestic detector - `rig.camera_red` → Red detector - `rig.cuttlefish_fip` → Controller device -- `data_stream_metadata[*].regions` → Camera identifiers for patch cord configurations. Streams without `regions` (data from aind-physiology-fip before 0.3.5) are filled from `rig.roi_settings` before validation, with a warning; if that is also null the mapper raises `ValueError`. +- `data_stream_metadata[*].regions` → Camera identifiers for patch cord configurations. Streams without `regions` (data from aind-physiology-fip before 0.3.5) fall back to `rig.roi_settings`, with a warning; if that is also null the mapper raises `ValueError`. The mapper assumes each ROI is drawn over its index-matched patch cord (ROI 0 over Patch Cord 0, and so on). Patch cords are created for each implanted fiber from the procedures record; the ROI settings only identify which cameras had ROIs drawn, and regions from all streams are merged by key. diff --git a/src/aind_metadata_mapper/fip/mapper.py b/src/aind_metadata_mapper/fip/mapper.py index 17863098..6b83c981 100644 --- a/src/aind_metadata_mapper/fip/mapper.py +++ b/src/aind_metadata_mapper/fip/mapper.py @@ -345,10 +345,6 @@ def transform( If metadata validation fails, or if a provided ethics_review_id does not match the IACUC protocol on record in LabTracks for the subject. """ - # Fill in per-stream `regions` from `rig.roi_settings` for data recorded before - # aind-physiology-fip 0.3.5, so one shape flows through validation and mapping (#498) - metadata = self._normalize_legacy_roi_settings(metadata) - # Validate metadata against JSON schema unless skipped if not skip_validation: self._validate_fip_metadata(metadata) @@ -361,8 +357,7 @@ def transform( subject_id = session["subject"] instrument_id = rig["rig_name"] - # Merge each stream's ROI settings by key; only the keys (which cameras had ROIs) are used - roi_settings = {k: v for ds in data_streams for k, v in ds["regions"].items()} + roi_settings = self._get_roi_settings(data_streams, rig) ethics_review_id = self._resolve_ethics_review_id(subject_id, ethics_review_id) @@ -502,15 +497,14 @@ def _extract_camera_exposure_time(self, rig_config: Dict) -> float: ) return 10000.0 # Default exposure time in microseconds - def _normalize_legacy_roi_settings(self, metadata: dict) -> dict: - """Fill in per-stream ``regions`` from ``rig.roi_settings`` for legacy input. + def _get_roi_settings(self, data_streams: List[Dict[str, Any]], rig_config: Dict[str, Any]) -> Dict[str, Any]: + """Merge the ROI settings used by all data streams, by key. - aind-physiology-fip 0.3.5 and later record the ROI settings used by each data stream - under ``regions``. Older data only has ``rig.roi_settings``, the definition loaded - before the experiment, so it is copied into any stream lacking ``regions`` (with a - warning). Streams that already have ``regions`` are left as is. The input is not - modified; metadata without data streams is returned unchanged for schema validation - to report. + Streams record the ROI settings in effect while they were collected under ``regions`` + (aind-physiology-fip 0.3.5 and later). Older data only has ``rig.roi_settings``, the + definition loaded before the experiment, which is used as a fallback with a warning. + Only the keys are consumed downstream (which cameras had ROIs drawn), so differences in + ROI position, size, or count between streams are not distinguished. Raises ------ @@ -519,11 +513,12 @@ def _normalize_legacy_roi_settings(self, metadata: dict) -> dict: aind-data-schema requires a PatchCordConfig for the fiber photometry modality, and those are built from the ROI settings, so the acquisition cannot be created. """ - rig_roi_settings = (metadata.get("rig") or {}).get("roi_settings") - streams = [] - for stream in metadata.get("data_stream_metadata") or []: - if not stream.get("regions"): - if not rig_roi_settings: + roi_settings: Dict[str, Any] = {} + for stream in data_streams: + regions = stream.get("regions") + if not regions: + regions = rig_config.get("roi_settings") + if not regions: raise ValueError( f"Data stream '{stream.get('id')}' has no regions and rig.roi_settings is missing or " "null in the extracted FIP metadata. Patch cord configurations are built from the ROI " @@ -534,9 +529,8 @@ def _normalize_legacy_roi_settings(self, metadata: dict) -> dict: f"Data stream '{stream.get('id')}' has no regions; falling back to rig.roi_settings. " "Per-stream ROI settings are recorded by aind-physiology-fip >= 0.3.5." ) - stream = {**stream, "regions": rig_roi_settings} - streams.append(stream) - return {**metadata, "data_stream_metadata": streams} if streams else metadata + roi_settings.update(regions) + return roi_settings def _get_camera_names_from_roi(self, roi_settings: Dict) -> Dict[str, str]: """Get camera device identifiers from ROI settings. diff --git a/tests/fip/test_mapper.py b/tests/fip/test_mapper.py index 2c83f72a..030904f2 100644 --- a/tests/fip/test_mapper.py +++ b/tests/fip/test_mapper.py @@ -631,10 +631,10 @@ def test_rig_roi_settings_ignored_when_regions_present(self): self.assertEqual(self._transform({**self.example_intermediate_data, "rig": bad_rig}), baseline) def test_legacy_streams_fall_back_to_rig_roi_settings(self): - """Test that streams without regions are filled from rig.roi_settings before validation. + """Test that streams without regions fall back to rig.roi_settings. Legacy data (aind-physiology-fip before 0.3.5) must validate, map identically to the - new shape, log a warning, and leave the caller's input untouched. + new shape, and log a warning. """ baseline = self._transform(self.example_intermediate_data) stream = self.example_intermediate_data["data_stream_metadata"][0] @@ -646,7 +646,6 @@ def test_legacy_streams_fall_back_to_rig_roi_settings(self): acquisition = self._transform(data, skip_validation=False) self.assertEqual(acquisition, baseline) self.assertIn("falling back to rig.roi_settings", logs.output[0]) - self.assertNotIn("regions", legacy_stream) def test_regions_merged_across_streams(self): """Test that camera identifiers are collected from the regions of every data stream.""" From 34e7f455507d8a90b99904f0047984cfc1eb2a08 Mon Sep 17 00:00:00 2001 From: Doug Ollerenshaw Date: Mon, 12 Oct 2026 18:19:13 -0700 Subject: [PATCH 3/4] Simplifying to derive camera names from rig config, not ROI settings --- src/aind_metadata_mapper/fip/README.md | 6 +- src/aind_metadata_mapper/fip/constants.py | 2 - src/aind_metadata_mapper/fip/mapper.py | 65 ++----------- tests/fip/test_mapper.py | 109 +++++++--------------- tests/fixtures/fip_intermediate.json | 78 +--------------- 5 files changed, 48 insertions(+), 212 deletions(-) diff --git a/src/aind_metadata_mapper/fip/README.md b/src/aind_metadata_mapper/fip/README.md index 2b1e20a6..afa52750 100644 --- a/src/aind_metadata_mapper/fip/README.md +++ b/src/aind_metadata_mapper/fip/README.md @@ -51,7 +51,7 @@ acquisition.write_standard_file(output_directory=Path("/output/path"), suffix=No The mapper expects input JSON with this structure: - `session` - experiment metadata (subject, experimenters, notes, etc.) - `rig` - hardware configuration (cameras, LEDs, ROI settings, etc.) -- `data_stream_metadata` - timing information and the ROI settings (`regions`) used in each stream +- `data_stream_metadata` - timing information ## What Gets Mapped @@ -69,9 +69,9 @@ The mapper expects input JSON with this structure: - `rig.camera_green_iso` → Green/Isosbestic detector - `rig.camera_red` → Red detector - `rig.cuttlefish_fip` → Controller device -- `data_stream_metadata[*].regions` → Camera identifiers for patch cord configurations. Streams without `regions` (data from aind-physiology-fip before 0.3.5) fall back to `rig.roi_settings`, with a warning; if that is also null the mapper raises `ValueError`. +- `rig.roi_settings` → Not currently read; may be null. Camera identifiers come from `rig.camera_*` and patch cords from the implanted fibers in the procedures record. The per-stream `data_stream_metadata[*].regions` recorded by aind-physiology-fip 0.3.5 and later are accepted but not read yet either. -The mapper assumes each ROI is drawn over its index-matched patch cord (ROI 0 over Patch Cord 0, and so on). Patch cords are created for each implanted fiber from the procedures record; the ROI settings only identify which cameras had ROIs drawn, and regions from all streams are merged by key. +The mapper assumes each ROI is drawn over its index-matched patch cord (ROI 0 over Patch Cord 0, and so on), that both cameras are in use, and that every implanted fiber has an active patch cord. Using the per-stream `regions` to determine which patch cords were actually in use is future work. ### Channels Each fiber gets three channels: diff --git a/src/aind_metadata_mapper/fip/constants.py b/src/aind_metadata_mapper/fip/constants.py index a7f1d6de..55055a8f 100644 --- a/src/aind_metadata_mapper/fip/constants.py +++ b/src/aind_metadata_mapper/fip/constants.py @@ -28,8 +28,6 @@ ROI_KEYWORD_GREEN = "green" ROI_KEYWORD_ISO = "iso" ROI_KEYWORD_RED = "red" -ROI_KEYWORD_ROI = "_roi" -ROI_KEYWORD_BACKGROUND = "_background" # ============================================================================== # Default Values diff --git a/src/aind_metadata_mapper/fip/mapper.py b/src/aind_metadata_mapper/fip/mapper.py index 6b83c981..35c0ffad 100644 --- a/src/aind_metadata_mapper/fip/mapper.py +++ b/src/aind_metadata_mapper/fip/mapper.py @@ -46,11 +46,9 @@ EXCITATION_UV, EXCITATION_YELLOW, FIP_CAMERA_COMPRESSION, - ROI_KEYWORD_BACKGROUND, ROI_KEYWORD_GREEN, ROI_KEYWORD_ISO, ROI_KEYWORD_RED, - ROI_KEYWORD_ROI, ) from aind_metadata_mapper.utils import ( ensure_timezone, @@ -357,8 +355,6 @@ def transform( subject_id = session["subject"] instrument_id = rig["rig_name"] - roi_settings = self._get_roi_settings(data_streams, rig) - ethics_review_id = self._resolve_ethics_review_id(subject_id, ethics_review_id) # Get timing from all data streams (handle multiple epochs) @@ -425,7 +421,7 @@ def transform( modalities=[Modality.FIB], code=code, active_devices=self._get_active_devices(rig, implanted_fibers), - configurations=self._build_configurations(rig, implanted_fibers, roi_settings, intended_measurements), + configurations=self._build_configurations(rig, implanted_fibers, intended_measurements), connections=self._build_connections(implanted_fibers), ) @@ -497,51 +493,16 @@ def _extract_camera_exposure_time(self, rig_config: Dict) -> float: ) return 10000.0 # Default exposure time in microseconds - def _get_roi_settings(self, data_streams: List[Dict[str, Any]], rig_config: Dict[str, Any]) -> Dict[str, Any]: - """Merge the ROI settings used by all data streams, by key. - - Streams record the ROI settings in effect while they were collected under ``regions`` - (aind-physiology-fip 0.3.5 and later). Older data only has ``rig.roi_settings``, the - definition loaded before the experiment, which is used as a fallback with a warning. - Only the keys are consumed downstream (which cameras had ROIs drawn), so differences in - ROI position, size, or count between streams are not distinguished. + def _get_camera_names(self, rig_config: Dict) -> Dict[str, str]: + """Get camera device identifiers from the rig configuration. - Raises - ------ - ValueError - If a stream has no ``regions`` and ``rig.roi_settings`` is also missing or null. - aind-data-schema requires a PatchCordConfig for the fiber photometry modality, and - those are built from the ROI settings, so the acquisition cannot be created. - """ - roi_settings: Dict[str, Any] = {} - for stream in data_streams: - regions = stream.get("regions") - if not regions: - regions = rig_config.get("roi_settings") - if not regions: - raise ValueError( - f"Data stream '{stream.get('id')}' has no regions and rig.roi_settings is missing or " - "null in the extracted FIP metadata. Patch cord configurations are built from the ROI " - "settings, and aind-data-schema requires a PatchCordConfig for the fiber photometry " - "modality, so the acquisition cannot be created without them." - ) - logger.warning( - f"Data stream '{stream.get('id')}' has no regions; falling back to rig.roi_settings. " - "Per-stream ROI settings are recorded by aind-physiology-fip >= 0.3.5." - ) - roi_settings.update(regions) - return roi_settings - - def _get_camera_names_from_roi(self, roi_settings: Dict) -> Dict[str, str]: - """Get camera device identifiers from ROI settings. - - Extracts camera config keys from ROI settings and transforms them to + Classifies the rig's camera entries as green or red and transforms them to historical standard device names using DEVICE_NAME_MAP. Parameters ---------- - roi_settings : Dict - ROI settings from rig configuration. + rig_config : Dict + Rig configuration dictionary from metadata. Returns ------- @@ -551,11 +512,8 @@ def _get_camera_names_from_roi(self, roi_settings: Dict) -> Dict[str, str]: """ camera_names = {} - for roi_key in roi_settings.keys(): - if ROI_KEYWORD_ROI in roi_key and ROI_KEYWORD_BACKGROUND not in roi_key: - # Extract camera key from roi_key (e.g., "camera_green_iso_roi" -> "camera_green_iso") - camera_key = roi_key.replace(ROI_KEYWORD_ROI, "") - + for camera_key in rig_config.keys(): + if camera_key.startswith("camera_"): # Determine camera type if ROI_KEYWORD_GREEN in camera_key or ROI_KEYWORD_ISO in camera_key: camera_type = "green" @@ -662,7 +620,6 @@ def _build_configurations( self, rig_config: Dict[str, Any], implanted_fibers: List[int], - roi_settings: Dict[str, Any], intended_measurements: Optional[Dict[str, Dict[str, Optional[str]]]] = None, ) -> List[Any]: """Build device configurations from rig config. @@ -671,8 +628,6 @@ def _build_configurations( ---------- rig_config : Dict[str, Any] Rig configuration dictionary from metadata. - roi_settings : Dict[str, Any] - ROI settings merged across data streams; identifies which cameras had ROIs drawn. intended_measurements : Optional[Dict[str, Dict[str, Optional[str]]]], optional Intended measurements from metadata service, mapping fiber names to channel measurements (R, G, B, Iso), by default None. @@ -704,8 +659,8 @@ def _build_configurations( # 1. Green: 470nm excitation, ~520nm emission, green camera # 2. Isosbestic: 415nm excitation, ~520nm emission, green camera (same detector!) # 3. Red: 565nm excitation, ~590nm emission, red camera - # Get camera identifiers from ROI settings - camera_names = self._get_camera_names_from_roi(roi_settings) + # Get camera identifiers from the rig configuration + camera_names = self._get_camera_names(rig_config) green_camera_name = camera_names.get("green") red_camera_name = camera_names.get("red") diff --git a/tests/fip/test_mapper.py b/tests/fip/test_mapper.py index 030904f2..575719e3 100644 --- a/tests/fip/test_mapper.py +++ b/tests/fip/test_mapper.py @@ -42,7 +42,6 @@ def setUp(self): } self.test_implanted_fibers = [0, 1] # Two implanted fibers self.test_ethics_review_id = ["2414"] # Injected to keep tests network-free - self.test_regions = self.example_intermediate_data["data_stream_metadata"][0]["regions"] # transform always calls get_iacuc_protocol to verify ethics_review_id against # LabTracks, so mock it here to keep all tests network-free. Individual tests @@ -51,16 +50,6 @@ def setUp(self): self.mock_get_iacuc_protocol = patcher.start() self.addCleanup(patcher.stop) - def _transform(self, data, **overrides): - """Run transform on ``data`` with network-free test inputs, overridable per call.""" - kwargs = dict( - skip_validation=True, - intended_measurements=self.test_intended_measurements, - implanted_fibers=self.test_implanted_fibers, - ethics_review_id=self.test_ethics_review_id, - ) - return self.mapper.transform(data, **{**kwargs, **overrides}) - def test_mapper_initialization(self): """Test that FIPMapper can be instantiated with default configuration. @@ -217,7 +206,6 @@ def test_code_field_from_rig_version(self): "id": "test_stream", "start_time": "2025-07-18 19:32:35.275046+00:00", "end_time": "2025-07-18 19:49:22.448358+00:00", - "regions": self.test_regions, } ], "session": { @@ -397,6 +385,40 @@ def test_configurations_built(self): self.assertGreater(len(patch_cords[0].channels), 0) self.assertIsNotNone(patch_cords[0].channels[0].detector) + def test_roi_settings_not_used(self): + """Test that ROI settings do not affect the acquisition. + + Camera identifiers come from the rig configuration and patch cords from the implanted + fibers, so rig.roi_settings may be null or absent (#498), and the per-stream regions + recorded by aind-physiology-fip 0.3.5 and later are accepted but not yet read. All + variants are run with schema validation on. + """ + data = self.example_intermediate_data + rig, stream = data["rig"], data["data_stream_metadata"][0] + variants = { + "baseline": data, + "rig.roi_settings null": {**data, "rig": {**rig, "roi_settings": None}}, + "rig.roi_settings absent": {**data, "rig": {k: v for k, v in rig.items() if k != "roi_settings"}}, + "stream regions present": {**data, "data_stream_metadata": [{**stream, "regions": rig["roi_settings"]}]}, + } + acquisitions = {} + for label, variant in variants.items(): + with self.subTest(variant=label): + acquisitions[label] = self.mapper.transform( + variant, + skip_validation=False, + intended_measurements=self.test_intended_measurements, + implanted_fibers=self.test_implanted_fibers, + ethics_review_id=self.test_ethics_review_id, + ) + for label, acquisition in acquisitions.items(): + self.assertEqual(acquisition, acquisitions["baseline"], label) + + def test_get_camera_names_ignores_unclassified_cameras(self): + """Test that camera entries that are neither green nor red are skipped.""" + names = self.mapper._get_camera_names({"camera_green_iso": {}, "camera_red": {}, "camera_other": {}}) + self.assertEqual(names, {"green": "Green CMOS", "red": "Red CMOS"}) + def test_timezone_handling(self): """Test that session times are properly timezone-aware after processing. @@ -463,19 +485,16 @@ def test_multiple_epochs_uses_earliest_and_latest_times(self): "id": "fip_epoch_0", "start_time": t0.isoformat(), "end_time": t1.isoformat(), - "regions": self.test_regions, }, { "id": "fip_epoch_1", "start_time": t2.isoformat(), "end_time": t3.isoformat(), - "regions": self.test_regions, }, { "id": "fip_epoch_2", "start_time": t4.isoformat(), "end_time": t5.isoformat(), - "regions": self.test_regions, }, ], "session": { @@ -605,66 +624,6 @@ def test_transform_missing_implanted_fibers_in_procedures(self): # Restore original method self.mapper._parse_implanted_fibers = original_method - def test_transform_fails_when_no_roi_settings(self): - """Test the clear ValueError when a stream has no regions and rig.roi_settings is also missing (#498).""" - stream = self.example_intermediate_data["data_stream_metadata"][0] - legacy_stream = {k: v for k, v in stream.items() if k != "regions"} - rig = {k: v for k, v in self.example_intermediate_data["rig"].items() if k != "roi_settings"} - cases = ( - ("null", {**legacy_stream, "regions": None}, {**rig, "roi_settings": None}), - ("absent", legacy_stream, rig), - ) - for label, bad_stream, bad_rig in cases: - with self.subTest(fields=label): - data = {**self.example_intermediate_data, "data_stream_metadata": [bad_stream], "rig": bad_rig} - with self.assertRaises(ValueError) as cm: - self._transform(data) - self.assertIn(stream["id"], str(cm.exception)) - self.assertIn("rig.roi_settings", str(cm.exception)) - - def test_rig_roi_settings_ignored_when_regions_present(self): - """Test that rig.roi_settings has no effect when every stream has regions (they take precedence).""" - baseline = self._transform(self.example_intermediate_data) - rig = {k: v for k, v in self.example_intermediate_data["rig"].items() if k != "roi_settings"} - for label, bad_rig in (("null", {**rig, "roi_settings": None}), ("absent", rig)): - with self.subTest(roi_settings=label): - self.assertEqual(self._transform({**self.example_intermediate_data, "rig": bad_rig}), baseline) - - def test_legacy_streams_fall_back_to_rig_roi_settings(self): - """Test that streams without regions fall back to rig.roi_settings. - - Legacy data (aind-physiology-fip before 0.3.5) must validate, map identically to the - new shape, and log a warning. - """ - baseline = self._transform(self.example_intermediate_data) - stream = self.example_intermediate_data["data_stream_metadata"][0] - legacy_stream = {k: v for k, v in stream.items() if k != "regions"} - for label, old_stream in (("null", {**legacy_stream, "regions": None}), ("absent", legacy_stream)): - with self.subTest(regions=label): - data = {**self.example_intermediate_data, "data_stream_metadata": [old_stream]} - with self.assertLogs(mapper_mod.logger, level="WARNING") as logs: - acquisition = self._transform(data, skip_validation=False) - self.assertEqual(acquisition, baseline) - self.assertIn("falling back to rig.roi_settings", logs.output[0]) - - def test_regions_merged_across_streams(self): - """Test that camera identifiers are collected from the regions of every data stream.""" - stream = self.example_intermediate_data["data_stream_metadata"][0] - green_only = {k: v for k, v in self.test_regions.items() if k != "camera_red_roi"} - red_only = {k: v for k, v in self.test_regions.items() if k != "camera_green_iso_roi"} - - def detectors(streams): - """Return the detector names across all patch cord channels for the given streams.""" - configs = self._transform({**self.example_intermediate_data, "data_stream_metadata": streams}) - patch_cords = [c for c in configs.data_streams[0].configurations if type(c).__name__ == "PatchCordConfig"] - return {ch.detector.device_name for pc in patch_cords for ch in pc.channels} - - self.assertEqual(detectors([{**stream, "regions": green_only}]), {"Green CMOS"}) - self.assertEqual( - detectors([{**stream, "id": "a", "regions": green_only}, {**stream, "id": "b", "regions": red_only}]), - {"Green CMOS", "Red CMOS"}, - ) - def test_validate_fip_metadata_error(self): """Test that _validate_fip_metadata raises ValueError on validation failure. diff --git a/tests/fixtures/fip_intermediate.json b/tests/fixtures/fip_intermediate.json index ef7f832b..e4432f22 100644 --- a/tests/fixtures/fip_intermediate.json +++ b/tests/fixtures/fip_intermediate.json @@ -3,83 +3,7 @@ { "id": "fip_2025-07-18T192959", "start_time": "2025-07-18 12:32:35.275046-07:00", - "end_time": "2025-07-18 12:49:22.448358-07:00", - "regions": { - "camera_green_iso_background": { - "center": { - "x": 10.0, - "y": 10.0 - }, - "radius": 10.0 - }, - "camera_red_background": { - "center": { - "x": 10.0, - "y": 10.0 - }, - "radius": 10.0 - }, - "camera_green_iso_roi": [ - { - "center": { - "x": 50.0, - "y": 50.0 - }, - "radius": 20.0 - }, - { - "center": { - "x": 50.0, - "y": 150.0 - }, - "radius": 20.0 - }, - { - "center": { - "x": 150.0, - "y": 50.0 - }, - "radius": 20.0 - }, - { - "center": { - "x": 150.0, - "y": 150.0 - }, - "radius": 20.0 - } - ], - "camera_red_roi": [ - { - "center": { - "x": 50.0, - "y": 50.0 - }, - "radius": 20.0 - }, - { - "center": { - "x": 50.0, - "y": 150.0 - }, - "radius": 20.0 - }, - { - "center": { - "x": 150.0, - "y": 50.0 - }, - "radius": 20.0 - }, - { - "center": { - "x": 150.0, - "y": 150.0 - }, - "radius": 20.0 - } - ] - } + "end_time": "2025-07-18 12:49:22.448358-07:00" } ], "session": { From 3c9a086d3e40afd30113d967b758eb48229e544e Mon Sep 17 00:00:00 2001 From: Doug Ollerenshaw Date: Mon, 12 Oct 2026 18:36:38 -0700 Subject: [PATCH 4/4] Update readme to be more explicit about assumptions --- src/aind_metadata_mapper/fip/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/aind_metadata_mapper/fip/README.md b/src/aind_metadata_mapper/fip/README.md index afa52750..0e0f78a6 100644 --- a/src/aind_metadata_mapper/fip/README.md +++ b/src/aind_metadata_mapper/fip/README.md @@ -71,7 +71,7 @@ The mapper expects input JSON with this structure: - `rig.cuttlefish_fip` → Controller device - `rig.roi_settings` → Not currently read; may be null. Camera identifiers come from `rig.camera_*` and patch cords from the implanted fibers in the procedures record. The per-stream `data_stream_metadata[*].regions` recorded by aind-physiology-fip 0.3.5 and later are accepted but not read yet either. -The mapper assumes each ROI is drawn over its index-matched patch cord (ROI 0 over Patch Cord 0, and so on), that both cameras are in use, and that every implanted fiber has an active patch cord. Using the per-stream `regions` to determine which patch cords were actually in use is future work. +The mapper assumes each ROI is drawn over its index-matched patch cord (ROI 0 over Patch Cord 0, and so on), that both cameras are in use, and that every implanted fiber has an active patch cord. The implanted fiber list comes from the subject's procedures record via the metadata service (see `_parse_implanted_fibers`), which describes the surgery rather than what was plugged in during the session. If the rig recorded which patch cords were in use and which fiber each was connected to, patch cord configurations and connections could be built from that record instead. That would be more accurate, and simpler: the procedures lookup (`_parse_implanted_fibers`, `get_procedures`), the `Fiber_` name parsing (`_extract_fiber_index`), the implanted-fiber error handling in `transform`, and the index-matching assumption behind `_build_connections` and `_get_active_devices` would all go away, with connections read from the rig record rather than synthesized. The per-stream `regions` recorded by aind-physiology-fip 0.3.5 and later do not yet carry that mapping (see [Aind.Physiology.Fip #40](https://github.com/AllenNeuralDynamics/Aind.Physiology.Fip/issues/40)). ### Channels Each fiber gets three channels: