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

Intrinsic signal imaging #10

Merged
merged 11 commits into from
Oct 9, 2024
Merged

Intrinsic signal imaging #10

merged 11 commits into from
Oct 9, 2024

Conversation

pauladkisson
Copy link
Member

@pauladkisson pauladkisson commented Oct 4, 2024

This PR adds intrinsic signal imaging, which is used to guide the ephys probe placement

@pauladkisson pauladkisson mentioned this pull request Oct 4, 2024
37 tasks
Base automatically changed from update_behavior to main October 9, 2024 15:47
@pauladkisson pauladkisson marked this pull request as ready for review October 9, 2024 15:48
Copy link

@h-mayorquin h-mayorquin left a comment

Choose a reason for hiding this comment

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

LGTM.

Do you think it would be a good idea to add devices that you mention in the description?

I wonder if there is value to it.

name: intrinsic_signal_optical_imaging
description: For precise targeting of auditory cortex, intrinsic optical imaging (IOS) will be performed using a 2-photon microscope (Neurolabware). The skull is first bilaterally thinned over a region of interest (ROI) and made translucent. On experiment day, 680nm red light (ThorLabs) is used to image the ROI. Data is collected via MATLAB running custom suites for online and offline analyses.
Images:
name: images

Choose a reason for hiding this comment

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

This does not follow the naming convention for the types, right?

https://nwbinspector.readthedocs.io/en/dev/best_practices/general.html#naming-conventions

Is that intentional? I am missing something?

Same for the processed image

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

I don't have a horse in this race but maybe it is important for you to know (or remember?) that when we discussed this in the LBNL - CN meeting Ben was very strong against changing the best practice and we did not come to any conclusion about it but Ryan promised to write some modified best practices. Maybe something to bring up again.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good to know, thx.

@pauladkisson
Copy link
Member Author

Do you think it would be a good idea to add devices that you mention in the description?

Yes, thanks for the suggestion.

@pauladkisson pauladkisson merged commit 1ee065e into main Oct 9, 2024
@pauladkisson pauladkisson deleted the intrinsic_signal_imaging branch October 9, 2024 22:23
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.

2 participants