-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ophys devices from ndx-ophys-devices #22
base: main
Are you sure you want to change the base?
Conversation
* update to new template * add ndx-ophys-devices as requirement * Import ndx-ophys-devices first * minor fixes --------- Co-authored-by: rly <[email protected]>
@pauladkisson Sorry for the messy PR. I'll try to explain what I did here:
Basically, I am asking you to review the following files that contain those 3 changes:
All the other files have already been reviewed in #26, where we updated the base template for the extension and fixed the import of ndx-ophys-device (#23). In a follow-up PR, I will make the ExcitationLightPath, the EmissionLightPath, and the ImagingSpace inherit from a general NWBContainer instead of LabMetadata. In other PR I will update the documentation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good! I made a few suggestions but nothing major.
I think it would help me a lot to have a quick meeting on this to make sure I understand what's going on (esp. with the newer types). I marked some questions ZoomQ for that purpose.
- neurodata_type_def: MicroscopyLightSource | ||
neurodata_type_inc: Device | ||
doc: Light source used to illuminate an imaging space. | ||
- neurodata_type_def: ExcitationLightPath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zoomQ: ExcitationLightPath is new to me, could you explain what its purpose is?
- name: description | ||
doc: Description or other notes about the channel. | ||
dtype: text | ||
- neurodata_type_def: EmissionLightPath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ZoomQ: EmissionLightPath is new to me, could you explain what its purpose is?
- name: emission_filter | ||
target_type: OpticalFilter | ||
doc: Link to OpticalFilter object which contains metadata about the optical filter in this emission light path. It can be either a BandOpticalFilter (e.g., 'Bandpass', 'Bandstop', 'Longpass', 'Shortpass') or a EdgeOpticalFilter (Longpass or Shortpass). | ||
quantity: '?' | ||
|
||
- neurodata_type_def: ImagingSpace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This basically replaces ImagingPlane right?
- name: emission_filter | ||
target_type: OpticalFilter | ||
doc: Link to OpticalFilter object which contains metadata about the optical filter in this emission light path. It can be either a BandOpticalFilter (e.g., 'Bandpass', 'Bandstop', 'Longpass', 'Shortpass') or a EdgeOpticalFilter (Longpass or Shortpass). | ||
quantity: '?' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a dichroic mirror in the light path of these microscopes? If so we should probably include it right?
|
||
|
||
def test_constructor_microscope(): | ||
ndx_microscopy.testing.mock_Microscope() | ||
mock_Microscope() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests really should have some assertions to check that the information is propagating properly.
For example, for the microscope,
def test_constructor_microscope():
microscope = mock_Microscope()
assert microscope.description == "A mock instance of a Microscope type to be used for rapid testing."
imaging_space=imaging_space, | ||
optical_channel=optical_channel, | ||
emission_light_path=emission_light_path, | ||
) | ||
nwbfile.add_acquisition(nwbdata=planar_microscopy_series) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if you add a PlanarMicroscopySeries to acquisition, but forget to add one of its devices?
I ask bc I've made this mistake and sometimes it fails ungracefully (ex. write works but read fails).
In this PR I added neurodatatypes from ndx-ophys-devices:
Entity relationship diagram for ndx-ophys-devices objects in ndx-microscopy
Entity relationship diagram for ndx-microscopy