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

[Question]: Assigning two PhotonSeries to the same ImagingPlane #575

Closed
h-mayorquin opened this issue Sep 19, 2023 · 6 comments · Fixed by #594
Closed

[Question]: Assigning two PhotonSeries to the same ImagingPlane #575

h-mayorquin opened this issue Sep 19, 2023 · 6 comments · Fixed by #594

Comments

@h-mayorquin
Copy link
Collaborator

h-mayorquin commented Sep 19, 2023

The metadata for the TwoPhotonSeries and the ImagingPlane are lists (as we can have more than one).

Example from the current Clandinin conversion:

metadata["Ophys"]["TwoPhotonSeries"]
[{'name': 'FunctionalGreenTwoPhotonSeries', 'description': 'Imaging data acquire...ng readout', 'unit': 'px', 'imaging_plane': 'ImagingPlane', 'dimension': [...], 'format': '.nii', 'scan_line_rate': 15833.056254848874, 'field_of_view': [...]}, {'name': 'FunctionalRedTwoPhotonSeries', 'description': 'Imaging data acquire...istration.', 'unit': 'px', 'imaging_plane': 'ImagingPlane', 'dimension': [...], 'format': '.nii', 'scan_line_rate': 15833.056254848874, 'field_of_view': [...]}]
len(metadata["Ophys"]["TwoPhotonSeries"])
2

When writing a conversion with multiple interfaces we use the conversion options in BaseImagingExtractorInterface to specify which metadata belongs to a specific interface:

def add_to_nwbfile(
self,
nwbfile: NWBFile,
metadata: Optional[dict] = None,
photon_series_type: Literal["TwoPhotonSeries", "OnePhotonSeries"] = "TwoPhotonSeries",
photon_series_index: int = 0,
stub_test: bool = False,
stub_frames: int = 100,
):

The argument is called photon_series_index and is used here:

photon_series_kwargs = metadata_copy["Ophys"][photon_series_type][photon_series_index]
photon_series_name = photon_series_kwargs["name"]

To my surprise, I realized that the the same index is used to determine which metadata is used to build the corresponding image_plane_index:

nwbfile = add_imaging_plane(nwbfile=nwbfile, metadata=metadata_copy, imaging_plane_index=photon_series_index)
imaging_plane_name = photon_series_kwargs["imaging_plane"]

Which, if I understand well, means that if I want to assign TwoPhotonSeries to the same imaging series I need to duplicated my metadata.


Maybe this work as intended and each TwoPhotonSeries should have ony ImagingPlane assigned? I think this is the decision that we did for Brucker.

I think this all comes from my confusion about the intent behind the ImagingPlane. When I first heard about volumetric data I was expecting that that would need multiple ImagingPlanes, one for every dimension of depth but this turned out to be false. On the other hand, I kind of expected that for formats with multiple channels (like Brucker) we could save one imagine plane with two TwoPhotonSeries referencing it and each TwoPhotonSeries somehow linked to their respective OpticalChannel. However, it seems that there is no way of doing this, as there is no way to directly link OpticalChannel back to the TwoPhotonSeries...

This is strange because OpticalChannel as an argument of ImagingPlane has a list type which I thought was meant to be used for ImagingPlanes with multiple channels but then how to disambiguate which series belongs to which channels? TwoPhotonSeries can only store one channel, either images or volumetric but not multiple channels.

@h-mayorquin
Copy link
Collaborator Author

It is also confusing for me that the indicator is in the ImagingPlane but that the emission_lambda is a property of the optical channel.

To prove the last point in my first comment, this works:

from pynwb.testing.mock.file import mock_NWBFile
from pynwb.testing.mock.ophys import mock_OpticalChannel
from pynwb.testing.mock.device import mock_Device
from pynwb.ophys import ImagingPlane

nwbfile = mock_NWBFile()
device = mock_Device(name='test_device')
test_optical_channel = mock_OpticalChannel(name='test_optical_channel')
another_optical_channel = mock_OpticalChannel(name='another_optical_channel')

optical_channel = [test_optical_channel, another_optical_channel]
imaging_plane = ImagingPlane(name="imaging_plane", description="", device=device, optical_channel=optical_channel, excitation_lambda=253.0, location="here", indicator="indicator")
imaging_plane.optical_channel

Output:

[test_optical_channel pynwb.ophys.OpticalChannel at 0x140450769060528
 Fields:
   description: description
   emission_lambda: 500.0,
 another_optical_channel pynwb.ophys.OpticalChannel at 0x140450720627008
 Fields:
   description: description
   emission_lambda: 500.0]

@weiglszonja
Copy link
Contributor

@h-mayorquin Propagating the imaging_plane_index in add_imaging_plane() to the add_photon_series you're right is not an ideal solution, this is why the TODO was added there to eventually change this:

# TODO: change imaging_plane_index to photon_series_key
nwbfile = add_imaging_plane(nwbfile=nwbfile, metadata=metadata_copy, imaging_plane_index=photon_series_index)
imaging_plane_name = photon_series_kwargs["imaging_plane"]

So basically add_imaging_plane would instead have "photon_series_key" where you can specify the name of the photon series you want to add the imaging plane for.

I have a different example that we can think about; for the Pinto lab we have the raw and the motion corrected OnePhotonSeries data. I would use the same imaging plane, but currently as you noticed it could only be done if I would duplicate the metadata. I'm not sure in that case having a "photon_series_key" would solve the problem.

@CodyCBakerPhD
Copy link
Member

It is also confusing for me that the indicator is in the ImagingPlane but that the emission_lambda is a property of the optical channel.

Another great point that @alessandratrapani can add to their ongoing list of improvements to implement to our ophys modalities 😃

@CodyCBakerPhD
Copy link
Member

@h-mayorquin Can this issue be closed? It's technically possible as @weiglszonja pointed out, even if not that ideal of a design (but that improvement is already a part of some older issues

@CodyCBakerPhD CodyCBakerPhD changed the title Assign two PhotonSeries to the same ImagingPlane [Question]: Assigning two PhotonSeries to the same ImagingPlane Sep 30, 2023
@weiglszonja
Copy link
Contributor

@h-mayorquin @CodyCBakerPhD Now to think of it, how would you feel about changing imaging_plane_index to imaging_plane_name? add_imaging_plane is used in segmentation as well,

add_imaging_plane(nwbfile=nwbfile, metadata=metadata_copy, imaging_plane_index=plane_segmentation_index)

so "photon_series_key" might be misleading there.
(I started drafting for #581 and realised if I wanted to test if we have raw and processed (e.g. downsampled) two photon series data that we add to acquisition and processing it could only be done by duplicating the metadata as @h-mayorquin pointed out)

@CodyCBakerPhD
Copy link
Member

Yes, the plan is always to swap from indexing of a list to string key identifier of a dictionary

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 a pull request may close this issue.

3 participants