Refactor write segmentation I #601

h-mayorquin · 2022-07-20T16:21:44Z

This is part of a join effort with @weiglszonja to refactor write_segmentation in the roiextractors module. This first PR does a couple of things:

It refactors the previously added function called add_imaging_plane and adds some unit tests.
The add_imaging_plane function is then used to refactor the write_segmentation function with some code reduction.
There are some other minor refactoring in documentation (add_device function) and style here and there.

…plane

h-mayorquin · 2022-07-21T08:02:51Z

src/nwb_conversion_tools/tools/roiextractors/roiextractors.py

@@ -38,6 +39,20 @@ def get_default_ophys_metadata():
    metadata = get_default_nwbfile_metadata()

    default_device = dict(name="Microscope")
+    default_optical_channel = dict(


This is just to flatten the metadata construction.

h-mayorquin · 2022-07-21T08:03:11Z

src/nwb_conversion_tools/tools/roiextractors/roiextractors.py

@@ -124,7 +123,12 @@ def get_nwb_imaging_metadata(imgextractor: ImagingExtractor):



Flattening ends here.

h-mayorquin · 2022-07-21T08:06:51Z

src/nwb_conversion_tools/tools/roiextractors/roiextractors.py

+        imaging_plane = _create_imaging_plane_from_metadata(
+            nwbfile=nwbfile, imaging_plane_metadata=imaging_plane_metadata
+        )
+        nwbfile.add_imaging_plane(imaging_plane)

    return nwbfile



The diff is doing a terrible job at displaying this but from line 141 to here the code displays the substitution of the old private method _add_imaging_plane for a new add_imaging_plane which adds the imaging plane to an nwb file and a second private function _create_imaging_plane_from_metadata which creates an image plane from the metadata when its name does not exist in the nwbfile yet.

I like this approach :)

Yes, I think that makes sense for this module. Having an auxiliary private function that creates the pynwb objects from the metadata and then a public tested function that adds this to the nwb file checking for edge cases and controlling parameters/options/etc.

src/nwb_conversion_tools/tools/roiextractors/roiextractors.py

weiglszonja · 2022-07-21T08:51:57Z

tests/test_internals/test_roi_extractors_module.py

+        second_imaging_plane_description = "second_imaging_plane_description"
+        self.imaging_plane_metadata["name"] = second_imaging_plane_name
+        self.imaging_plane_metadata["description"] = second_imaging_plane_description
+        add_imaging_plane(nwbfile=self.nwbfile, metadata=self.metadata)


I have a question here, there is an imaging_plane_index in add_imaging_plane which is 0 by default. When you add the second imaging plane here, are you using the same imaging_plane_index? (Maybe I don't get what an imaging_plane_index is for)

Yes, I think this is confusing (at least it confused me) and I think it stems from a design decision that was made about our metadata format. In order to mimic the nwb schema the metadata for imagine plane is stored at the top level of ophys as a list. The reason for this, as far as I understand, is that the imaging_plane in the nwb-schema is a group of objects. Each of the imaging plane in this group are then stored as links in all the objects that require them (e.g. TwoPhotonSeries). I think that the decision was make to make our own schema in nwb-conversion tools mimic as much as possible the true schema. Therefore, the metadata for imaging_plane is a list at the top level of our metadata.

The crucial fact is that the metadata for the imaging_plane in our metadata dict is a list and therefore an specification of which element/index of the list do we want to add becomes necessary. That's the role of imagine_plane_index. If you see the previous code that this is removing it was assumed that the element was always the first element of the list which I think works right now because we never had a conversion with more than one plane. I changed this to make this assumption explicit and make it a parameter that can be changed if it is required.

Maybe there is a better name? as we don't have examples of conversion data with more than two imaging planes I don't know what's the best way to handle this.

Now, the case when the segmentation extractor is an MultiSegmentationExtractor object would seem to be this case that I mention is lacking as it was meant to handle segmentation objects each with a different imaging plane (see catalystneuro/roiextractors#178).

But strangely, the decision was make to instead pass a list of metadata dictionaries (as you can see at the beginning of the code in the function) and therefore the metadata for image_list is still a list with only one element and the code that assumes that the first element of the list is the corresponding metadata of the image list still works.

I see, thanks for clarifying it.

h-mayorquin · 2022-07-21T09:57:52Z

@CodyCBakerPhD it seems that the windows tests are failing again even with re-running:

https://github.com/catalystneuro/nwb-conversion-tools/runs/7446674056?check_suite_focus=true
https://github.com/catalystneuro/nwb-conversion-tools/runs/7446339098?check_suite_focus=true

…plane

codecov · 2022-07-21T11:43:11Z

Codecov Report

Merging #601 (29b2bcf) into main (9a9fb75) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #601      +/-   ##
==========================================
+ Coverage   88.34%   88.38%   +0.04%     
==========================================
  Files          59       59              
  Lines        3218     3221       +3     
==========================================
+ Hits         2843     2847       +4     
+ Misses        375      374       -1

Flag	Coverage Δ
unittests	`88.38% <100.00%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...b_conversion_tools/tools/roiextractors/__init__.py	`100.00% <ø> (ø)`
...version_tools/tools/roiextractors/roiextractors.py	`80.63% <100.00%> (+0.63%)`	⬆️

CodyCBakerPhD · 2022-07-21T15:34:02Z

@h-mayorquin @weiglszonja FYI this stuff did not make it over to https://github.com/catalystneuro/neuroconv prior to commit I used to split so this chain of PRs will have to be ported over

added tests for add_imaging_plane and refactor

4e9bdfb

h-mayorquin added testing refactoring writing for the long term labels Jul 20, 2022

h-mayorquin requested a review from weiglszonja July 20, 2022 16:21

h-mayorquin self-assigned this Jul 20, 2022

h-mayorquin added 3 commits July 20, 2022 18:21

Merge branch 'main' into refactor_write_segmentation_using_add_image_…

15eacd4

…plane

changed argument name of add_imaging_plane

abcda23

reposition add_imaging_plane in add_two_photon_series

b98a35e

This was referenced Jul 20, 2022

Refactor write segmentation II (use add_devices) #602

Merged

Refactor write segmentation III (add summary images) #603

Merged

h-mayorquin commented Jul 21, 2022

View reviewed changes

weiglszonja reviewed Jul 21, 2022

View reviewed changes

added docstrings to added functions in roiextractors module

db5527f

weiglszonja approved these changes Jul 21, 2022

View reviewed changes

Merge branch 'main' into refactor_write_segmentation_using_add_image_…

29b2bcf

…plane

h-mayorquin merged commit 3ce21b6 into main Jul 21, 2022

h-mayorquin deleted the refactor_write_segmentation_using_add_image_plane branch July 21, 2022 11:58

h-mayorquin mentioned this pull request Jul 21, 2022

Restablish last roi extractors pull requests catalystneuro/neuroconv#19

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor write segmentation I #601

Refactor write segmentation I #601

h-mayorquin commented Jul 20, 2022

h-mayorquin Jul 21, 2022

h-mayorquin Jul 21, 2022

h-mayorquin Jul 21, 2022

weiglszonja Jul 21, 2022

h-mayorquin Jul 21, 2022

weiglszonja Jul 21, 2022

h-mayorquin Jul 21, 2022 •

edited

Loading

h-mayorquin Jul 21, 2022 •

edited

Loading

weiglszonja Jul 21, 2022

h-mayorquin commented Jul 21, 2022 •

edited

Loading

codecov bot commented Jul 21, 2022

CodyCBakerPhD commented Jul 21, 2022

		@@ -124,7 +123,12 @@ def get_nwb_imaging_metadata(imgextractor: ImagingExtractor):

Refactor write segmentation I #601

Refactor write segmentation I #601

Conversation

h-mayorquin commented Jul 20, 2022

h-mayorquin Jul 21, 2022

Choose a reason for hiding this comment

h-mayorquin Jul 21, 2022

Choose a reason for hiding this comment

h-mayorquin Jul 21, 2022

Choose a reason for hiding this comment

weiglszonja Jul 21, 2022

Choose a reason for hiding this comment

h-mayorquin Jul 21, 2022

Choose a reason for hiding this comment

weiglszonja Jul 21, 2022

Choose a reason for hiding this comment

h-mayorquin Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

h-mayorquin Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

weiglszonja Jul 21, 2022

Choose a reason for hiding this comment

h-mayorquin commented Jul 21, 2022 • edited Loading

codecov bot commented Jul 21, 2022

Codecov Report

CodyCBakerPhD commented Jul 21, 2022

h-mayorquin Jul 21, 2022 •

edited

Loading

h-mayorquin Jul 21, 2022 •

edited

Loading

h-mayorquin commented Jul 21, 2022 •

edited

Loading