-
Notifications
You must be signed in to change notification settings - Fork 1
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
Ndx fiber photometry #24
Conversation
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.
General comments:
- Be consistent in naming all the variables and fields that refer to the isosbestic path. You call it either isosbestic or reference. I suggest going with isosbestic (I add a few Suggested Changes but I'll leave you the full revision).
- It was suggested to me on old reviews of my PRs to name all the objects in the NWBFile keeping the nomenclature of the class they refer to, e.g instead of "dms_signal_excitation_source" --> "ExcitationSourceSignal". I tried my best to suggest the changes for all the devices and others, but I missed something for sure. I'll let you correct the rest.
- The scope of this new extension was to share resources as much as possible. Try to avoid duplicating devices that share manufacturer and model.
src/lerner_lab_to_nwb/seiler_2024/seiler_2024fiberphotometryinterface.py
Show resolved
Hide resolved
src/lerner_lab_to_nwb/seiler_2024/seiler_2024fiberphotometryinterface.py
Outdated
Show resolved
Hide resolved
src/lerner_lab_to_nwb/seiler_2024/seiler_2024fiberphotometryinterface.py
Outdated
Show resolved
Hide resolved
src/lerner_lab_to_nwb/seiler_2024/seiler_2024fiberphotometryinterface.py
Outdated
Show resolved
Hide resolved
src/lerner_lab_to_nwb/seiler_2024/seiler_2024fiberphotometryinterface.py
Outdated
Show resolved
Hide resolved
src/lerner_lab_to_nwb/seiler_2024/seiler_2024fiberphotometryinterface.py
Outdated
Show resolved
Hide resolved
src/lerner_lab_to_nwb/seiler_2024/seiler_2024fiberphotometryinterface.py
Outdated
Show resolved
Hide resolved
src/lerner_lab_to_nwb/seiler_2024/seiler_2024fiberphotometryinterface.py
Outdated
Show resolved
Hide resolved
src/lerner_lab_to_nwb/seiler_2024/seiler_2024fiberphotometryinterface.py
Show resolved
Hide resolved
src/lerner_lab_to_nwb/seiler_2024/seiler_2024fiberphotometryinterface.py
Show resolved
Hide resolved
There is some open discussion on this: rly/ndx-pose#31 |
I see. I was using data objects to represent the physical devices (ex. 1 ExcitationSource object per different LED source). But it sounds like the idea is to collapse the metadata as much as possible, which is fine by me. |
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.
LGTM
Fixes #21
See example session