search

catalystneuro / roiextractors

Python-based module for extracting from, converting between, and handling optical imaging data from several file formats. Inspired by SpikeInterface.
https://roiextractors.readthedocs.io/en/latest/index.html
BSD 3-Clause "New" or "Revised" License
11 stars 7 forks source link

Fix bug in `BrukerTiffSinglePlaneImagingExtractor` #343

Closed h-mayorquin closed 2 months ago

h-mayorquin commented 2 months ago

The current code assumes that the channel name (the stream) should be in the filename. This is not the case for some data that I have. Here is a frame element in the correspoding xml

    <Frame relativeTime="0.00949076" absoluteTime="41.2114907600011" index="3" parameterSet="CurrentSettings">
      <File channel="1" channelName="Red" filename="TSeries-20240527-001_Cycle00001_Ch1_000003.ome.tif" />
      <File channel="2" channelName="Green" filename="TSeries-20240527-001_Cycle00001_Ch2_000003.ome.tif" />

As you can see the current asumption is not valid and therefore the code is a bug. The fix proposed in this PR yses the channelName in the xml element to filter the file paths that correspond to the channel.

h-mayorquin commented 2 months ago

Ah, OK, this is a case for a problem when using a single argument to specify plane and channel.

Here: https://github.com/catalystneuro/roiextractors/blob/10aea18c8abe2e70fb9ed163ba435293f66fde5a/src/roiextractors/extractors/tiffimagingextractors/brukertiffimagingextractor.py#L226-L230

We pass plane_stream (a string identifying the plane) as a stream to BrukerTiffSinglePlaneImagingExtractor. But in some other places we pass a channel. The current implementation is relying on the fact that both are included in the filename and that this code will work. However, as I argued above the channelName is not always in the filename.

I think at least at this level we should seprate them, it would make the implementation easier.

weiglszonja commented 2 months ago

Thank you @h-mayorquin for looking into this issue. I still think that (following this issue):

We refactor the code so the classes are clearly separated as you guys propose.

would be the way to go, it would follow how @pauladkisson refactored the extractor for ScanImage, I believe we don't have a routing class there either. We didn't know it was a bug because the examples that we've seen for Bruker always followed the convention we built on, I think it would be reasonable to have designated classes instead of a router.

h-mayorquin commented 2 months ago

Ok, @weiglszonja I found a way of solving this without requiring more changes. I think that the changes we have discussed are more robust and would make the code clear but this should be a intermediate fix. Note that this requires https://github.com/catalystneuro/roiextractors/pull/344 before merging.