[Bug] PlaneSegmentation can be initialized without image, pixel or voxel masks

[h-mayorquin]

The following code does not throw an error:

from pynwb.ophys import PlaneSegmentation, ImagingPlane, Device, OpticalChannel
from hdmf.common import VectorData

channel = OpticalChannel(name='test', description='test', emission_lambda=1.0)
device = Device(name='test', description='test')
imaging_plane = ImagingPlane(name='test', indicator='test', location='test', optical_channel=channel, description='test', device=device, excitation_lambda=1.3)

column = VectorData(data=[0, 1, 2, 3], name='test_vector', description='test_vector')
plane_segmentation = PlaneSegmentation(imaging_plane=imaging_plane, columns=[column], description="this")

But the following does:

plane_segmentation = PlaneSegmentation(imaging_plane=imaging_plane, description="this")
plane_segmentation.add_roi(id=0)

Trace:

ValueError                                Traceback (most recent call last)
Cell In [22], line 2
      1 plane_segmentation = PlaneSegmentation(imaging_plane=imaging_plane, description="this")
----> 2 plane_segmentation.add_roi(id=0)

File ~/miniconda3/envs/neuroconv_environment/lib/python3.9/site-packages/hdmf/utils.py:645, in docval.<locals>.dec.<locals>.func_call(*args, **kwargs)
    643 def func_call(*args, **kwargs):
    644     pargs = _check_args(args, kwargs)
--> 645     return func(args[0], **pargs)

File ~/miniconda3/envs/neuroconv_environment/lib/python3.9/site-packages/pynwb/ophys.py:255, in PlaneSegmentation.add_roi(self, **kwargs)
    253 pixel_mask, voxel_mask, image_mask = popargs('pixel_mask', 'voxel_mask', 'image_mask', kwargs)
    254 if image_mask is None and pixel_mask is None and voxel_mask is None:
--> 255     raise ValueError("Must provide 'image_mask' and/or 'pixel_mask'")
    256 rkwargs = dict(kwargs)
    257 if image_mask is not None:

ValueError: Must provide 'image_mask' and/or 'pixel_mask'

My understanding is that the first block of code should also thrown an error. Something else to do here is to add clear documentation to add_roi to indicate that at least one of 'image_mask', 'pixel_mask' or 'voxel_mask' is to be stored: https://pynwb.readthedocs.io/en/stable/pynwb.ophys.html#pynwb.ophys.PlaneSegmentation.add_roi

[bendichter]

@rly what do you think? Should we allow for now roi mask? @h-mayorquin has a use-case where no segmentation was done.

[rly]

@h-mayorquin Could you describe the use case further? I don't totally see the use of adding an ROI without any image mask, pixel mask, or voxel mask. Is the ROI defined using custom columns? Third-party tools already have to fork their behavior based on whether an image, pixel, or voxel mask is provided. So, if the ROI is defined in some other way using custom columns, I think it would probably be fine to amend the schema and API to allow none of those datasets to be defined.

[CodyCBakerPhD]

@h-mayorquin Could you describe the use case further? I don't totally see the use of adding an ROI without any image mask, pixel mask, or voxel mask.

It's an edge case where a single 'ROI' is defined by the experimenter and is essentially an image_mask that is equal to an array of equal weight at every pixel

[CodyCBakerPhD]

I.e., the related RoiResponseSeries is simply the summed activity of all the pixels in the TwoPhotonSeries over time

[rly]

I see - so the ROI is the entire image. I think that should be explicitly defined, and it is OK to have an image_mask with an array of equal weight at every pixel. Alternatively, we could:

1. add to the documentation that if none of image_mask, pixel_mask, or voxel_mask are provided, then the ROI is the entire image
2. add a boolean column "entire_image" to the table (or just to the method PlaneSegmentation.add_roi) that signals that the ROI is the entire image

[CodyCBakerPhD]

I think that should be explicitly defined, and it is OK to have an image_mask

I'm perfectly fine with this, too. It'll maintain compatibility with other more standard interpretations without adding any special conditions (this seems like a fairly rare case)

[oruebel]

I think that should be explicitly defined, and it is OK to have an image_mask with an array of equal weight at every pixel

I agree. I like that this is a very explicit solution and maintains compatibility without introducing special cases.

[rly]

I think this was resolved in discussion here but please re-open if not.