Change `add_imaging_plane` to add imaging plane by name instead of index #594

weiglszonja · 2023-10-13T15:19:33Z

add imaging_plane_name keyword argument to add_imaging_plane function
find which imaging plane to add from the metadata by name instead of index
in add_photon_series and add_plane_segmentation the name of the imaging plane is identified from the metadata of photon series or plane segmentation
add imaging_plane to default plane segmentation metadata
resolve [Question]: Assigning two PhotonSeries to the same ImagingPlane #575

…name instead of index

h-mayorquin

This looks very good to me and makes sense.

No major comments other than minor suggestions.

src/neuroconv/tools/roiextractors/roiextractors.py

tests/test_ophys/test_tools_roiextractors.py

Co-authored-by: Heberto Mayorquin <[email protected]>

for more information, see https://pre-commit.ci

# Conflicts: # CHANGELOG.md

h-mayorquin

This looks great. I am wondering if it would make sense to also get the photon_series properties by name instead of by index as well.

Just some final suggestions about the tests and a small question.

h-mayorquin · 2023-10-17T12:59:50Z

src/neuroconv/tools/roiextractors/roiextractors.py

    imaging_plane = nwbfile.get_imaging_plane(name=imaging_plane_name)
+    photon_series_kwargs = deepcopy(photon_series_metadata)


Why was this needed?
My memory is that we needed to deepcopy the metadata so we don't modify the data that the user passes but the hpoton_series_metadata is already deepcopied so why copy it again?

To avoid mutating the other copy I assume 😅

yeah, sorry I answered in a separate reply ..

You're right, the reason why I chose this is to avoid overwriting the "imaging_plane" in the metadata with the actual ImagingPlane object at this line.

Yes and it would definitely make sense to eventually also use photon_series_name as identifier instead of index!

src/neuroconv/tools/roiextractors/roiextractors.py

tests/test_ophys/test_tools_roiextractors.py

h-mayorquin · 2023-10-17T13:09:36Z

tests/test_ophys/test_tools_roiextractors.py

@@ -1427,6 +1469,25 @@ def test_add_one_photon_series_roundtrip(self):
            expected_one_photon_series_shape = (self.num_frames, self.num_columns, self.num_rows)
            assert one_photon_series.shape == expected_one_photon_series_shape

+    def test_add_multiple_one_photon_series_with_same_imaging_plane(self):
+        """Test adding two OnePhotonSeries that use the same ImagingPlane."""
+        add_photon_series(


What do you think of making this explicit by explictly setting the:

first_photon_series_metadata = deepcopy(self.one_photon_series_metadata) second_photon_series_metadata = deepcopy(self.one_photon_series_metadata) first_photon_series_metadata["Ophys"]["OnePhotonSeries"][0]["imaging_plane"] = "same_imaging_plane_for_two_series" second_photon_series_metadata["Ophys"]["OnePhotonSeries"][0]["imaging_plane"] = "same_imaging_plane_for_two_series"

Or we could use a single metadata dictionary for the two of them as well...

explicit is always better, but I'm not sure how you imagine the single metadata dictionary, something like this:

neuroconv/tests/test_ophys/test_tools_roiextractors.py

Lines 1474 to 1480 in 3c17f24

shared_photon_series_metadata = deepcopy(self.one_photon_series_metadata)

shared_imaging_plane_name = "same_imaging_plane_for_two_series"

shared_photon_series_metadata["Ophys"]["ImagingPlane"][0]["name"] = shared_imaging_plane_name

shared_photon_series_metadata["Ophys"]["OnePhotonSeries"][0]["imaging_plane"] = shared_imaging_plane_name

add_photon_series(

3c17f24

So,
metadata["Ophys"]["OnePhotonSeries"] will be a list with two elements (one per plane). Then you will need to specificy the photon_series_index for each call to add_photon_series

But if we are moving towards #269 then maybe is not worth to do it like this.

@h-mayorquin are you imagining for this PR to include the change in the metadata schema? If not, I think here is fine to assume the list structure?

No, not all, I am saying that if you don't want to have a single metadata dictionary that is totally fine. Because, when we eventually do #269 then we will need to change the list structure anyway.

So up to you.

h-mayorquin · 2023-10-17T13:11:54Z

tests/test_ophys/test_tools_roiextractors.py

+        )
+
+        self.assertIn("second_photon_series", self.nwbfile.acquisition)
+


Do you think it would be a good idea to check that we only have one imaging plane here written?

Something like

assert len(self.nwbfile.imaging_planes) == 0 assert self.nwbfile.imaging_planes[0]["name"] == "the_default_name_or_the_name_we_chosen_in_this_test"

The second line is probably wrong but you get the idea.

Yes, we should add a check for that! Thanks

weiglszonja · 2023-10-17T13:51:40Z

src/neuroconv/tools/roiextractors/roiextractors.py

    imaging_plane = nwbfile.get_imaging_plane(name=imaging_plane_name)
+    photon_series_kwargs = deepcopy(photon_series_metadata)
    photon_series_kwargs.update(imaging_plane=imaging_plane)


@h-mayorquin

My memory is that we needed to deepcopy the metadata so we don't modify the data that the user passes but the hpoton_series_metadata is already deepcopied so why copy it again?

You're right, the reason why I chose this is to avoid overwriting the "imaging_plane" in the metadata with the actual ImagingPlane object at this line.

Ah, OK, probably not necessary anymore then.

CodyCBakerPhD · 2023-10-17T14:14:15Z

First steps towards #269

codecov · 2023-10-18T09:02:17Z

Codecov Report

Merging #594 (8aeeeb8) into main (32df08a) will increase coverage by 0.01%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #594      +/-   ##
==========================================
+ Coverage   90.98%   91.00%   +0.01%     
==========================================
  Files         103      103              
  Lines        5369     5378       +9     
==========================================
+ Hits         4885     4894       +9     
  Misses        484      484

Flag	Coverage Δ
unittests	`91.00% <100.00%> (+0.01%)`	⬆️

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

Files	Coverage Δ
src/neuroconv/tools/roiextractors/roiextractors.py	`89.14% <100.00%> (+0.29%)`	⬆️

h-mayorquin

I think this is done right?

weiglszonja · 2023-10-18T12:17:25Z

Yes, I think it's finished.
There is one failing CI test but I think it is not related to these changes.
Should I merge?

h-mayorquin · 2023-10-18T12:26:07Z

Yes, that's fine, I think the current one that is failing I fixed but another one seems to have appeared.

weiglszonja added 3 commits October 13, 2023 17:07

add imaging plane link to default segmentation metadata

4188c97

change logic to find which imaging plane to add from the metadata by …

9137e77

…name instead of index

add tests

06474bd

weiglszonja marked this pull request as draft October 13, 2023 15:19

Merge branch 'main' into change_imaging_plane_identifier_to_string

4a2db06

weiglszonja self-assigned this Oct 13, 2023

weiglszonja added 2 commits October 13, 2023 17:48

add test DeprecationWarning when index is used

23af231

update CHANGELOG.md

044bcd5

weiglszonja marked this pull request as ready for review October 16, 2023 09:48

weiglszonja requested a review from h-mayorquin October 16, 2023 09:48

h-mayorquin reviewed Oct 16, 2023

View reviewed changes

weiglszonja and others added 7 commits October 16, 2023 14:20

Apply suggestions from code review

8ab2182

Co-authored-by: Heberto Mayorquin <[email protected]>

[pre-commit.ci] auto fixes from pre-commit.com hooks

8181646

for more information, see https://pre-commit.ci

add suggestions from review

d1f86a0

fix valueerror typo

611366c

Merge branch 'main' into change_imaging_plane_identifier_to_string

e3b10f0

fix tests

a2ee93b

Merge branch 'main' into change_imaging_plane_identifier_to_string

80fab88

# Conflicts: # CHANGELOG.md

weiglszonja mentioned this pull request Oct 17, 2023

Update Suite2pSegmentationInterface to support multiple channels and planes #601

Merged

5 tasks

h-mayorquin reviewed Oct 17, 2023

View reviewed changes

weiglszonja commented Oct 17, 2023

View reviewed changes

add check for shared imaging plane

3c17f24

Merge branch 'main' into change_imaging_plane_identifier_to_string

8aeeeb8

h-mayorquin approved these changes Oct 18, 2023

View reviewed changes

h-mayorquin merged commit ec748f1 into main Oct 18, 2023
33 of 34 checks passed

h-mayorquin deleted the change_imaging_plane_identifier_to_string branch October 18, 2023 12:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change `add_imaging_plane` to add imaging plane by name instead of index #594

Change `add_imaging_plane` to add imaging plane by name instead of index #594

weiglszonja commented Oct 13, 2023

h-mayorquin left a comment

h-mayorquin left a comment

h-mayorquin Oct 17, 2023

CodyCBakerPhD Oct 17, 2023

weiglszonja Oct 17, 2023

weiglszonja Oct 18, 2023

h-mayorquin Oct 17, 2023

weiglszonja Oct 17, 2023

h-mayorquin Oct 17, 2023 •

edited

Loading

weiglszonja Oct 17, 2023

h-mayorquin Oct 17, 2023

h-mayorquin Oct 17, 2023

weiglszonja Oct 17, 2023

weiglszonja Oct 17, 2023 •

edited

Loading

h-mayorquin Oct 17, 2023

CodyCBakerPhD commented Oct 17, 2023

codecov bot commented Oct 18, 2023

h-mayorquin left a comment

weiglszonja commented Oct 18, 2023 •

edited

Loading

h-mayorquin commented Oct 18, 2023

		imaging_plane = nwbfile.get_imaging_plane(name=imaging_plane_name)
		photon_series_kwargs = deepcopy(photon_series_metadata)

	shared_photon_series_metadata = deepcopy(self.one_photon_series_metadata)
	shared_imaging_plane_name = "same_imaging_plane_for_two_series"

	shared_photon_series_metadata["Ophys"]["ImagingPlane"][0]["name"] = shared_imaging_plane_name
	shared_photon_series_metadata["Ophys"]["OnePhotonSeries"][0]["imaging_plane"] = shared_imaging_plane_name

	add_photon_series(

		)

		self.assertIn("second_photon_series", self.nwbfile.acquisition)

Change add_imaging_plane to add imaging plane by name instead of index #594

Change add_imaging_plane to add imaging plane by name instead of index #594

Conversation

weiglszonja commented Oct 13, 2023

h-mayorquin left a comment

Choose a reason for hiding this comment

h-mayorquin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-mayorquin Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiglszonja Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodyCBakerPhD commented Oct 17, 2023

codecov bot commented Oct 18, 2023

Codecov Report

h-mayorquin left a comment

Choose a reason for hiding this comment

weiglszonja commented Oct 18, 2023 • edited Loading

h-mayorquin commented Oct 18, 2023

Change `add_imaging_plane` to add imaging plane by name instead of index #594

Change `add_imaging_plane` to add imaging plane by name instead of index #594

h-mayorquin Oct 17, 2023 •

edited

Loading

weiglszonja Oct 17, 2023 •

edited

Loading

weiglszonja commented Oct 18, 2023 •

edited

Loading