-
Notifications
You must be signed in to change notification settings - Fork 28
Propagate CaImAn quality metrics #1436
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
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Hi, see comments on #1414 (comment) to see how to move forward with this. Feel free to reach me in slack for a call if things are not clear. |
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.
We discussed the following points on a meeting.
@@ -950,6 +980,12 @@ def _add_plane_segmentation( | |||
data=is_id_rejected, | |||
) | |||
|
|||
if include_quality_metrics and quality_metrics is not None: |
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.
default_descriptions = {"snr": "signal_to_noise_ratio_orwhatever", "other_property": "default_description}"
If your property is named like that then you used this description, otherwise, you use empty string or no description.
src/neuroconv/datainterfaces/ophys/caiman/caimandatainterface.py
Outdated
Show resolved
Hide resolved
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.
The CaImAn part is ready, we need the generic test now.
@@ -823,6 +843,7 @@ def add_plane_segmentation_to_nwbfile( | |||
include_roi_acceptance=include_roi_acceptance, | |||
mask_type=mask_type, | |||
iterator_options=iterator_options, | |||
quality_metrics=quality_metrics, |
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.
We should call this segmentation_extractor_properties because it is not only quality metrics.
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.
done
@@ -950,6 +969,13 @@ def _add_plane_segmentation( | |||
data=is_id_rejected, | |||
) | |||
|
|||
# Always add quality metrics if they are available |
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 think the default definitions should be here:
quality_metrics_definitions = {
"snr": "Signal-to-noise ratio for each component",
"r_values": "Spatial correlation values for each component",
"cnn_preds": "CNN classifier predictions for component quality",
}
close to where they are used.
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.
done.
@@ -536,6 +536,26 @@ def test_rejected_roi_ids(self, rejected_list, expected_rejected_roi_ids): | |||
plane_segmentation_accepted_roi_ids = plane_segmentation["Accepted"].data | |||
assert_array_equal(plane_segmentation_accepted_roi_ids, accepted_roi_ids) | |||
|
|||
def test_add_plane_segmentation_with_quality_metrics(self): | |||
"""Test that available quality metrics in the segmentation extractor are added as columns to the PlaneSegmentation when available.""" |
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 looks fishy,
if & is intersection I think this test will always pass. or am I wrong?
Anyway, for these test here we have absolute control. Can you add some specific properties to the segmentation extractor here and test:
- that they are added
- their values match what we just added.
So just create a segmentation extractor, add properties with values that you control of various dtypes and check that they are written correctly.
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.
done
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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1436 +/- ##
==========================================
+ Coverage 87.04% 87.06% +0.01%
==========================================
Files 145 145
Lines 9681 9691 +10
==========================================
+ Hits 8427 8437 +10
Misses 1254 1254
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Fix #1414 : propagate quality metrics and add multiplane stub files to testing.