Skip to content
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

Merged
merged 12 commits into from
May 23, 2024
Merged

Ndx fiber photometry #24

merged 12 commits into from
May 23, 2024

Conversation

pauladkisson
Copy link
Member

@pauladkisson pauladkisson commented May 2, 2024

Fixes #21

See example session

@pauladkisson pauladkisson mentioned this pull request May 2, 2024
24 tasks
@pauladkisson pauladkisson marked this pull request as ready for review May 6, 2024 19:15
Copy link

@alessandratrapani alessandratrapani left a comment

Choose a reason for hiding this comment

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

General comments:

  1. 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).
  2. 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.
  3. The scope of this new extension was to share resources as much as possible. Try to avoid duplicating devices that share manufacturer and model.

@pauladkisson
Copy link
Member Author

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.

There is some open discussion on this: rly/ndx-pose#31
And given all the considerations mentioned on that issue, I think snake_case is the more appropriate/intuitive choice for naming, so I'm going to stick with it for this conversion.

@pauladkisson
Copy link
Member Author

pauladkisson commented May 21, 2024

The scope of this new extension was to share resources as much as possible. Try to avoid duplicating devices that share manufacturer and model.

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.

Copy link

@alessandratrapani alessandratrapani left a comment

Choose a reason for hiding this comment

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

LGTM

@pauladkisson pauladkisson merged commit 4ce8212 into main May 23, 2024
@pauladkisson pauladkisson deleted the ndx_fiber_photometry branch May 23, 2024 18:36
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.

Switch to ndx-fiber-photometry
2 participants