search

catalystneuro / nwb-conversion-tools

Create NWB files by converting and combining neural data in proprietary formats and adding essential metadata.
https://nwb-conversion-tools.readthedocs.io/en/main/
BSD 3-Clause "New" or "Revised" License
25 stars 12 forks source link

Discussion: transposing images in `roiextractors` #590

Closed weiglszonja closed 2 years ago

weiglszonja commented 2 years ago

ROIExtractors read and return image shapes that corresponds to (height x width), whereas NWB expects in (width x height). Currently, we transpose image data in add_two_photon_series as:

https://github.com/catalystneuro/nwb-conversion-tools/blob/a8f224f9611a3b7444964f6d3b8b2e887a18a5f2/src/nwb_conversion_tools/tools/roiextractors/roiextractors.py#L185-L188 wrapped into DataChunkIterator. While working on #575, it caused some issues with determining the shape of the buffer, since there was a mismatch between the original image size (height x width) and data shape (transposed to width x height).

I would like to open a discussion about this, because with #575 being in progress I feel like there could be a more appropriate place manipulating the image data than the iterator level. What if the ImagingExtractors would make sure that the images are returned in the appropriate (width x height) format?

Let me know what you think about this, @CodyCBakerPhD @bendichter @h-mayorquin

(While it is connected to roiextractors, the place where the image data is being transposed is in CT, so this why I opened the issue here, but let me know if I should move this issue somewhere else.)

CodyCBakerPhD commented 2 years ago

What if the ImagingExtractors would make sure that the images are returned in the appropriate (width x height) format?

Unfortunately that would be a much bigger overhaul - also, it's more natural for ROIExtractors to use height x width since that is what several of the constituent formats use, and seems to be an ad-hoc 'standard' axis convention for image processing.

Basically, NWB is the outlier in that respect, so it makes the most sense to just order the axes when mapping from ROIExtractors to NWB.

It's also not that awkward to perform such axes mappings at the iterator level, we used to do that for the old spikeextractors (when traces returned channels x frames).

While working on https://github.com/catalystneuro/nwb-conversion-tools/pull/575, it caused some issues with determining the shape of the buffer, since there was a mismatch between the original image size (height x width) and data shape (transposed to width x height).

The axis order should not cause problems with size determination - if it's that extra unsqueezed problem you found in https://github.com/catalystneuro/fee-lab-to-nwb/issues/16, it should be unrelated to this. If it's a new problem, what exactly is the issue you encountered?

weiglszonja commented 2 years ago

@CodyCBakerPhD the maximum shape that is determined by the new iterator was incorrect, because it considers the original order of axes and not the transposed shape of data, i fixed it here:

https://github.com/catalystneuro/nwb-conversion-tools/blob/a253a5535aaa6123209912468798b788ddfc284a/src/nwb_conversion_tools/tools/hdmf.py#L160-L161

CodyCBakerPhD commented 2 years ago

the maximum shape that is determined by the new iterator was incorrect, because it considers the original order of axes and not the transposed shape of data, i fixed it here:

Right, this is why the new iterator acts more like an API map from whatever type of read structure you want to specify (so if adjusting axis order in the data retrieval step, those axis must match those specified by any _shape of the iterator.