Skip to content

Conversation

@dougollerenshaw
Copy link
Contributor

@dougollerenshaw dougollerenshaw commented Oct 16, 2025

Implements a FIP (Fiber Photometry) mapper that transforms intermediate metadata from the extractor into schema-compliant Acquisition objects.
Follows the SmartSPIM mapper pattern (transform-only, no I/O). Creates 3 channels per fiber (Green, Isosbestic, Red) reflecting the temporal multiplexing setup. Integrates with metadata service endpoints for intended measurements and procedures data to validate which fibers were actually implanted.
External API calls are mocked in tests so they run fast (~1.5s) and work in CI/CD. Falls back to ROI-based inference if service endpoints are unavailable.
TODO:
Camera exposure times (currently using -1 as placeholder)
Filter specifications
Full connection graph
All tests pass. Example script runs successfully.

Closes: https://github.com/AllenNeuralDynamics/aind-physio-arch/issues/811

- Implements FIPMapper class for transforming intermediate FIP metadata to Acquisition schema
- Follows SmartSPIM mapper pattern: extract → transform → save
- Maps FIP hardware configs (LEDs, cameras, patch cords, fibers) to schema objects
- Zero-indexed naming: ROI N → Patch Cord N → Fiber N (implanted)
- Includes comprehensive TODO documentation for future enhancements
- Adds unit tests with 100% passing coverage
- Includes working example script and sample acquisition.json output

Key features:
- Transforms session metadata (times, experimenter, subject, rig)
- Builds Channel objects with detectors and emission wavelengths
- Creates PatchCordConfig for each ROI/fiber pair
- Includes implanted fiber identifiers in active devices
- Handles timezone-aware timestamps
- Validates against aind-data-schema v2.0+

TODO items documented for future work:
- Connection graph (requires instrument metadata endpoint)
- Intended measurements (requires ROI mapping)
- Filter specifications (requires rig config enhancement)
- LED-to-ROI mapping (requires rig config enhancement)
@dougollerenshaw dougollerenshaw changed the base branch from dev to release-v1.0.0 October 16, 2025 02:41
…data

Follow standard AIND mapper pattern:
- Add run_job() method for one-step transform + write workflow
- Make output filename configurable at initialization (defaults to 'acquisition.json')
- Update write() to return Path and use mapper's output_filename attribute
- Example uses custom filename 'fip_example_acquisition.json' to avoid conflicts

Document FIP system architecture in detail:
- Each implanted fiber has 3 temporal-multiplexed channels (60Hz cycling)
- Green channel: 470nm excitation → ~520nm emission → Green CMOS
- Isosbestic channel: 415nm excitation → ~520nm emission → Green CMOS (same camera!)
- Red channel: 565nm excitation → ~590nm emission → Red CMOS
- Clarify current implementation creates 1-2 channels per fiber vs needed 3 channels
- Document missing pieces: LED-to-ROI mapping, ROI-to-measurement mapping

Add wavelength inference and metadata:
- New _get_led_wavelength() maps LED names to excitation wavelengths (415nm, 470nm, 565nm)
- Update _infer_emission_wavelength() with accurate values (520nm green/iso, 590nm red)
- Include wavelengths in LED device names (LED_UV_415nm, LED_BLUE_470nm, LED_LIME_565nm)
- Update emission wavelength from 525nm/600nm to 520nm/590nm based on system specs

Update documentation and examples:
- README shows both run_job() and transform()+write() patterns
- Note that output filename is configurable but defaults to standard 'acquisition.json'
- Example script demonstrates custom filename usage
- All tests pass (12/12)

This commit maintains backward compatibility while aligning with AIND mapper standards
and providing better documentation for future full 3-channel-per-fiber implementation.
…m metadata service

- Add requests import for HTTP calls to metadata service
- Implement get_intended_measurements() method to query intended_measurements endpoint
- Method queries http://aind-metadata-service/intended_measurements/{subject_id}
- Returns dictionary mapping Fiber_N to channel measurements (R, G, B, Iso)
- Handles HTTP errors gracefully with warnings
- Tested successfully with subject 726649

Next step: Integrate into channel creation logic to populate intended_measurement field
Changes:
- Call get_intended_measurements() in _transform() using subject_id
- Pass intended_measurements to _build_configurations()
- Add _get_channel_measurement() helper to map ROI+camera to measurement
- Populate channel intended_measurement field from metadata service
- Maps camera types: red camera→R channel, green camera→G channel
- Handles missing measurements gracefully (returns None)

Documentation:
- Add CURRENT IMPLEMENTATION section showing what's done
- Clean up TODO list to only show remaining work
- Remove completed items: intended_measurement, fiber IDs, LED wavelengths
- Focus TODO on: 3-channel structure, filters, connection graph, detailed device metadata

Successfully tested with subject 726649 (calcium, dopamine, control measurements).
Major changes:
- Now creates 3 channels per fiber: Green, Isosbestic, and Red
- Green and Isosbestic channels share same detector (green CMOS) but have different excitation wavelengths
- Each channel gets its own intended_measurement from metadata service (G, Iso, R)
- Channel names: Fiber_N_Green, Fiber_N_Isosbestic, Fiber_N_Red

Implementation details:
- Simplified patch cord creation logic to iterate over fibers, not cameras
- Each fiber gets all 3 channels if cameras are present
- Removed _get_channel_measurement() helper (no longer needed)
- Updated documentation to reflect 3-channel implementation

Testing:
- All 12 unit tests pass
- Successfully creates 3 channels per patch cord
- Example output verified with 4 fibers × 3 channels each = 12 total channels

This completes the channel structure implementation!
… physics

Changes:
- Define module-level constants for LED excitation wavelengths:
  * EXCITATION_UV = 415nm (UV LED)
  * EXCITATION_BLUE = 470nm (Blue LED)
  * EXCITATION_YELLOW = 565nm (Yellow/Lime LED)
- Define emission wavelength constants:
  * EMISSION_GREEN = 520nm (Green camera)
  * EMISSION_RED = 590nm (Red camera)
- Clarify excitation → emission physics throughout:
  * Blue LED (470nm) → green emission (520nm) [GFP signal]
  * UV LED (415nm) → green emission (520nm) [isosbestic control]
  * Yellow LED (565nm) → red emission (590nm) [RFP signal]
- Rename variables to reflect LED type (blue_led, uv_led, yellow_led)
- Update all docstrings to clearly explain excitation/emission relationship
- Replace all magic number references with named constants

Testing:
- All 12 unit tests pass
- No functional changes, purely refactoring for code clarity
Changes to mapper:
- Add get_implanted_fibers() method to fetch from procedures endpoint
  * Queries http://aind-metadata-service-dev/api/v2/procedures/{subject_id}
  * Parses Brain injection procedures to identify implanted fiber indices
  * Maps relative_position (Right→0, Left→1) to fiber indices
  * Timeout set to 60s due to slow endpoint (~30s response time)
- Update _build_configurations() to accept implanted_fibers parameter
  * Only creates PatchCordConfig objects for actually implanted fibers
  * Falls back to ROI-based inference if procedures data unavailable
  * Prevents creating connections to non-existent implanted fibers
- Update _get_active_devices() to accept implanted_fibers parameter
  * Only includes patch cords and fibers that actually exist
  * Consistent with patch cord configuration logic
- Update mapper documentation to reflect procedures integration

Changes to tests:
- Mock get_implanted_fibers() to return [0, 1] (two fibers)
- Mock get_intended_measurements() to return sample measurements
- Add setUp/tearDown methods to start/stop mocking
- Tests now run in ~1.3s instead of ~124s (100x faster!)
- Tests will work in GitHub CI/CD (no external service dependencies)

Testing:
- All 12 unit tests pass
- Falls back gracefully if procedures endpoint fails
- Maintains backwards compatibility with ROI-based fiber detection

This addresses the requirement to validate which fibers are actually
implanted using surgical procedure records, while keeping tests fast
and portable.
Changes:
- Add PLACEHOLDER_CAMERA_EXPOSURE_TIME = -1 constant at module level
  * Clearly indicates missing data (impossible negative exposure time)
  * Easy to find and replace when actual camera metadata is available
  * Well-documented with TODO comment
- Use constant in all three channel DetectorConfig objects:
  * Green channel (Blue LED → Green CMOS)
  * Isosbestic channel (UV LED → Green CMOS)
  * Red channel (Yellow LED → Red CMOS)
- Remove standalone DetectorConfig objects from configurations list
  * Now only created inline within Channel objects
  * Prevents duplication and inconsistency
- Update test to check for DetectorConfig within PatchCordConfig channels
  * Verifies proper nesting structure
- Update TODO documentation to reference the placeholder constant

Testing:
- All 12 unit tests pass
- Exposure time clearly marked as placeholder in code
- No fake/hallucinated values pretending to be real data

The -1 value makes it immediately obvious to anyone inspecting the
acquisition.json that this is placeholder data requiring replacement.
Changes:
- Corrected logic to look for 'Probe implant' procedures (not 'Brain injection')
- Check for implanted_device with object_type == 'Fiber probe'
- Extract fiber index from device name (e.g., 'Fiber_0' -> 0, 'Fiber_1' -> 1)
- Fixed indentation issues in _build_configurations fallback logic

This addresses the user's feedback that the previous implementation was
incorrectly mapping brain injection relative_position (Left/Right) to
fiber indices. The correct approach is to parse the actual Fiber probe
implant procedures and extract the fiber indices directly from the
device names.

Testing:
- All 12 unit tests pass
- Mock still returns [0, 1] for test consistency
- Real procedures endpoint would return indices like [0, 1, 2] based on
  actual 'Fiber_0', 'Fiber_1', 'Fiber_2' device names
@Ahad-Allen Ahad-Allen self-requested a review October 16, 2025 07:02
Copy link
Contributor

@Ahad-Allen Ahad-Allen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions, but mostly look fine to me



# FIP system LED excitation wavelengths (nm)
EXCITATION_UV = 415 # UV LED → green emission (isosbestic control)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having these values here is fine for now, but perhaps we should be pulling them from rig info in the extractor instead( if they are stored there at all)? It seems like this is not robust to changes to the rig and an easy source of miss-able errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. These are unlikely to change since they are attributes of the standard FIP hardware setup. But it's still not great to have them here. Maybe they're in the instrument.json? I just added a comment acknowledging that we should find a better source for these values, but i'm leaving them for now. 11ccc70

response = requests.get(url, timeout=5)

if response.status_code not in [200, 300]:
print(f"Warning: Could not fetch intended measurements for subject {subject_id} (status {response.status_code})")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should use logging here to follow the practices we're using for the extractor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. See b2df282


return result if result else None

except Exception as e:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to return none in cases where this fails? I feel like allowing it to error will lead to errors being caught faster. If there's a need to have this to always pass to allow data to be uploaded, I can understand the logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. This info will be missing if the experimenter didn't add it during the surgical request. So we don't want that to cause an error. But I did add a warning so it's not hidden: 49be169

fiber_idx = int(fiber_name.split('_')[1])
implanted_indices.add(fiber_idx)
except (IndexError, ValueError):
# Skip if we can't parse the fiber index
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar question here. Perhaps we want to have a more transparent error process of allowing the mapper to fail if there is an invalid fiber_{idx} name?

intended_measurements = self.get_intended_measurements(subject_id)
implanted_fibers = self.get_implanted_fibers(subject_id)

data_stream = DataStream(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is missing a list of connections?

return wavelength
return None

def _infer_emission_wavelength(self, camera_name: str) -> Optional[int]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is called anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! See 054e7e7.

- Add .pre-commit-config.yaml with black, isort, flake8, and basic checks
- Run black formatter on all FIP files
- Run isort to organize imports
- Fix line length issue in mapper.py
- Configure flake8 to ignore complexity warnings (C901)
- All tests passing after formatting changes
- Extract helper methods to reduce cyclomatic complexity
- Add _extract_fiber_index to simplify fiber parsing
- Add _get_fiber_indices_from_roi to deduplicate ROI logic
- Add _build_led_configs to separate LED configuration building
- Add _create_channel to reduce duplication in channel creation
- Remove C901 complexity warning ignore from pre-commit config
- All complexity warnings resolved, all tests passing
- Create utils.py with general mapper utilities
- Add write_acquisition() for generic acquisition writing
- Add get_procedures() to fetch from metadata service
- Add get_intended_measurements() to fetch from metadata service
- Update FIP mapper to use utilities and parse responses
- Rename methods to _parse_intended_measurements and _parse_implanted_fibers
- Remove hardcoded endpoint URLs from FIP mapper
- Update tests to mock new method names
- All tests passing, linters passing
- Move ensure_timezone from nested function to utils module
- Use system local timezone instead of hardcoding America/Los_Angeles
- Remove ZoneInfo and datetime imports from FIP mapper (no longer needed)
- Update docstring to clarify timezone behavior
- All tests passing
- Add logging module setup following extractor pattern
- Replace all print() with logger.info() and logger.warning()
- Use logger.info() for informational messages
- Use logger.warning() for warnings/errors
- Matches pattern used in aind-metadata-extractor
- All tests passing
Copy link
Contributor

@Ahad-Allen Ahad-Allen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes look good!

… FIP mapper\n- Log a clear warning when no intended_measurements are found for a subject\n- Continue gracefully with fields set to None\n- Tests passing
…nal and gate transform\n- Switch tests to fixture + SimpleNamespace stub via _transform\n- Add tests/fixtures/fip_intermediate.json\n- Keep CI green when extractor not installed
…_lime, detailed device configs, background ROIs\n- Include networking, session config details, version info\n- Update test expectations for realistic subject/rig IDs\n- All tests passing with more comprehensive fixture
…All tests now call _transform with SimpleNamespace stub\n- Tests work without extractor dependency\n- All 78 tests passing
… to tests/fip/test_mapper.py\n- Add utils tests to tests/test_utils.py\n- Consolidate all FIP mapper tests in one file\n- All 91 tests passing, 96% coverage
…olidate all FIP mapper tests into logical TestFIPMapper class\n- Remove ridiculous class names like 'TestFIPMapperCoverage' and 'TestFIPMapperImportError'\n- Group tests by functionality: basic mapping, field mapping, data streams, time handling, etc.\n- Add comprehensive docstrings to all test functions explaining purpose and expected behavior\n- Create separate TestFIPMapperEdgeCases class for edge case testing without setUp mocking\n- Achieve 100% coverage for both FIP mapper and utils with 41 tests passing in 1.21s\n- Tests now organized logically by business functionality rather than development order
… for dependency isolation and control\n- Document patch.object() for method replacement and call verification\n- Cover SimpleNamespace for fixture data without extractor dependency\n- Describe test class separation: core logic vs edge cases\n- Detail HTTP mocking strategy with response object configuration\n- Emphasize hermetic testing with temp directories and error handling focus
- Add docstring to TestUtils class to complete 99.5% -> 100% coverage
- All functions and classes now have comprehensive docstrings
- Delete tests/test_fip_mapper.py as tests were moved to tests/fip/test_mapper.py
- Tests now properly organized to match source code structure
- Create protocols.yaml to centralize protocol URLs by modality
- Add load_protocols() and get_protocols_for_modality() to utils
- Update FIP mapper to include protocol_id in Acquisition schema
- Add pyyaml dependency to pyproject.toml
- All tests passing (41/41)
- Add _extract_camera_exposure_time() method to extract delta_1 from light sources
- Delta_1 represents camera integration time in microseconds during LED pulses
- Convert microseconds to milliseconds for DetectorConfig
- Update _create_channel() to accept and use exposure_time_ms parameter
- Replace PLACEHOLDER_CAMERA_EXPOSURE_TIME (-1) with actual extracted values
- Add clear documentation explaining delta_1, delta_2, delta_3, delta_4 meanings
- No magic numbers: all constants clearly defined at module level
- All tests passing (41/41)
- Add tests/fip/__init__.py so unittest discover finds FIP tests
- Add protocol error path tests (missing file, invalid YAML)
- Add camera exposure missing delta_1 test
- All 110 tests passing with 100% coverage
- Consolidate two identical warning messages into one shorter message
- Second warning now more concise: 'No valid fiber measurements found'
- Simplify protocols.yaml format to avoid YAML parsing errors in CI
- Fix test_import_fip_data_model_success to properly mock the import path
- All 110 tests now passing locally
- Integrate .flake8 configuration file for consistent flake8 behavior
- Remove flake8 args from .pre-commit-config.yaml per review feedback
- Keep extractor dependency needed for FIP mapper functionality
- Resolve .flake8 conflict by taking release branch version
- Remove W503 from extend-ignore per release branch
- Keep exclude patterns and other settings
- Remove aind-metadata-extractor from optional dependencies
- Extractor package not available on PyPI yet
- FIP mapper handles missing extractor gracefully with ImportError
@dougollerenshaw dougollerenshaw changed the title fip mapper fip mapper for new standard Oct 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants