-
Notifications
You must be signed in to change notification settings - Fork 7
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 Minian segmentation extractor #368
base: main
Are you sure you want to change the base?
Conversation
Do tag me when this is ready for review. |
for more information, see https://pre-commit.ci
I am uploading on the s3 bucket the testing folder for the minian segmentation data |
@h-mayorquin ready for review |
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.
I took the first look. Looks good and simple to me.
- Is the structured documented? (the one that you shared on the PR) I am inferring the meaning of A, C, b, f, etc from the equation of CNMF here:
https://minian.readthedocs.io/en/stable/pipeline/notebook_5.html
But maybe there is other place in the documentation where this is stated more precisely, if so, we probably should link that in the docstring.
- The example that you uploaded, can you add a readme and share it with me so that I can upload it to gin.
class MinianSegmentationExtractor(SegmentationExtractor): | ||
"""A SegmentationExtractor for Minian. | ||
|
||
This class inherits from the SegmentationExtractor class, having all |
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.
I am not sure this docstring is very helpful, I think it should be oriented to explain things to final users and not implementation details.
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.
Ok, sure! I just wanted to be consistent with all the other extractors, but I guess a more detailed docstring won't hurt.
self.folder_path = folder_path | ||
self._roi_response_denoised = self._trace_extractor_read(field="C") | ||
self._roi_response_baseline = self._trace_extractor_read(field="b0") | ||
self._roi_response_neuropil = self._trace_extractor_read(field="f") |
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.
I don't understand how the "f" (the temporal activities of the background) is the response neuropill. Can you say how those concepts match?
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.
Let's say first that the background can contain neuropil signal. The problem is that I wanted to map this "f" signal with something already in the SegmentationExtractor
.
Now, I am not super happy with this formulation I think we should have two separate variables:
_roi_response_neuropil
with the respective_neuropil_images_masks
. This can be used to store the actual neuropil signals that sometimes refer to the area surrounding the ROIs_roi_response_background
with the respective_background_images_masks
. This can be used more generally to store the signal in the field of view that excludes the areas in the identified ROIs (as in this case).
I opened an issue on roiextractor to discuss this with the others: #375
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.
OK, makes sense.
Add segmentation extractor for Minian output.
Supported output format: Zarr (for now)
Similarly to Caiman uses CNMF to perform cell identification.
Output structure (example):
image_masks
roi_response_denoised
background_image_masks
roi_response_neuropil
roi_response_baseline
roi_response_deconvolved
summary_image
TODOs