Skip to content

Conversation

@samuelbray32
Copy link
Collaborator

@samuelbray32 samuelbray32 commented May 5, 2025

Fixes #110

Related PR's:

To Do:

  • FS gui spatial filters. Update extension for multiplefilters per epoch and polygonal filters
    • In progress. have nodes parsed and added just couple little entries left
  • Source and add epoch-level metadata
  • Load in "device" yamls for new objects. Propose generalizing "probe_metadata" to "device_metadata" and using it to passing around metadata for multiple device types (virus, excitation source, etc.)
  • Add tests
  • cameras defining spatial regions
    • Implemented in yaml generator but not yet in this package test yaml file

@codecov
Copy link

codecov bot commented May 6, 2025

Codecov Report

Attention: Patch coverage is 90.41096% with 21 lines in your changes missing coverage. Please review.

Project coverage is 89.50%. Comparing base (73e14e6) to head (539095c).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
src/trodes_to_nwb/convert_optogenetics.py 90.29% 20 Missing ⚠️
src/trodes_to_nwb/convert.py 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #118      +/-   ##
==========================================
+ Coverage   89.32%   89.50%   +0.17%     
==========================================
  Files          12       13       +1     
  Lines        1471     1686     +215     
==========================================
+ Hits         1314     1509     +195     
- Misses        157      177      +20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@samuelbray32 samuelbray32 marked this pull request as ready for review July 2, 2025 15:42
@edeno edeno requested a review from Copilot July 2, 2025 15:46

This comment was marked as outdated.

@samuelbray32 samuelbray32 requested review from Copilot and edeno July 2, 2025 15:51
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for ingesting and converting optogenetic metadata into the NWB export pipeline by generalizing “probe_metadata” to “device_metadata”, introducing a new convert_optogenetics module, and providing sample/test YAMLs and device metadata files.

  • Renamed and extended metadata loading to handle multiple device types (probes, viruses, fibers, light sources).
  • Added convert_optogenetics to parse and inject optogenetic devices, injections, fibers, and stimulation epochs.
  • Introduced test fixtures (fsgui_*.yaml, geometry.trackgeometry), updated sample metadata, and new unit tests for optogenetics.

Reviewed Changes

Copilot reviewed 19 out of 28 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/trodes_to_nwb/convert.py Swapped probe → device metadata paths, hooked in add_optogenetics
src/trodes_to_nwb/convert_optogenetics.py New module: builds optogenetic devices, fibers, viruses, epochs
src/trodes_to_nwb/tests/test_convert_optogenetics.py Unit tests validating optogenetic device and epoch creation
src/trodes_to_nwb/tests/test_data/fsgui_theta_trigger.yaml FS GUI YAML for theta-triggered optogenetic epochs
src/trodes_to_nwb/tests/test_data/20230622_sample_metadata.yml Added virus_injection, opto_excitation_source, optical_fiber, FS‐GUI YAML refs
Comments suppressed due to low confidence (2)

src/trodes_to_nwb/tests/test_data/fsgui_theta_trigger.yaml:1

  • The list items under nodes: are not indented, which will produce invalid YAML. Prefix each item with two spaces before - (e.g., - type_id: ...).
nodes:

src/trodes_to_nwb/tests/test_data/20230622_sample_metadata.yml:656

  • Inconsistent key casing: you have both volume_in_uL and volume_in_ul. Standardize to a single key (e.g., volume_in_uL) to avoid lookup errors.
    volume_in_ul: 100.0

edeno
edeno previously approved these changes Jul 2, 2025
Copy link
Collaborator

@edeno edeno left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I had a couple questions:

  1. What happens if an old user has an old yaml and uses the new version of trodes_to_nwb.
  2. Where does someone go to find documentation about the different opto objects and what they correspond to?

Copy link
Collaborator

@edeno edeno left a comment

Choose a reason for hiding this comment

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

Err actually the docstrings are in the google format and not the numpy docstring style. Would be nice to be consistent.

@samuelbray32
Copy link
Collaborator Author

I think the new pynwb=3.1.0 release is breaking. I might pin the version and solve in a separate PR

@samuelbray32 samuelbray32 requested a review from edeno July 9, 2025 18:26
@edeno edeno merged commit 883a2a4 into main Jul 9, 2025
10 checks passed
@edeno edeno deleted the opto branch July 9, 2025 20:41
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.

Optogenetic metadata

3 participants