flatironinstitute / CaImAn

Computational toolbox for large scale Calcium Imaging Analysis, including movie handling, motion correction, source extraction, spike deconvolution and result visualization.
https://caiman.readthedocs.io
GNU General Public License v2.0
640 stars 370 forks source link

Get correct centers of mass from rois.com even when swap_dim is True in get_contours #1370

Closed ethanbb closed 4 months ago

ethanbb commented 4 months ago

Description

The issue is that the rois.com function always assumes that each column of A is in F-order, but get_contours was not taking that into account when calling it with swap_dim=True.* In particular, plot_contours transposes the image and reverses the order of dimensions input to get_contours when swap_dim is true. This means that A is now in C-order relative to the dimensions.

This solution rewrites rois.com to take an order parameter. So when swap_dim is true, order should be 'C'.

Also, with this call fixed, it's unnecessary to call com again in plot_contours (I believe); the 'CoM' field of each coordinates dict can be used to plot the labels.

* (I doubt this option is commonly used, but I was using it for 3D CNMF because the way it handles 3D arrays (iterating over the first dimension) means it made more sense to used swapped dims so that it was iterating over planes. Not sure if this should also be changed, though.)

Type of change

Please delete options that are not relevant.

Has your PR been tested?

I added a test to test_toydata.py (not sure if this is a good place) to ensure that the contours and coms are the same regardless of orientation for the 2D case. (I don't think there's a straightforward way to swap_dim for the 3D case because of the way planes are handled when finding contours). caimanmanager test and caimanmanager demotest pass.

Also, I did some manual testing with the demo notebook. I had to cut the test movie in half along the x-axis to reproduce the bug because it only matters when the x and y sizes differ and the demo movie is square.

Before this change, with swap_dim = True (note that the label coordinates are incorrect):

Screenshot 2024-07-02 142337

After change with swap_dim = True - labels now line up with contours

After change with swap_dim = False:

ethanbb commented 4 months ago

While we're editing get_contours, I wonder if it would also make sense to iterate through the last dimension of B (i.e., z) than the first for the 3D case? I wasn't sure why it currently iterates through the first dimension, and I found a way to work around it, but if there isn't a good reason let's just change it?

(My assumption is that it makes sense to find contours on z planes because the x and y axes are roughly equally-spaced while the z axis is different/probably sparser - at least that is definitely true in my case.)

pgunn commented 4 months ago

I'm open to that get_countours change idea, although providing a parameter to choose may be a good idea.

ethanbb commented 4 months ago

OK, I made it so that by default, get_contours tries to slice along what should be the z axis depending on swap_dims (which is a change in behavior for swap_dims=False. But it can be overridden if needed.

I also made a couple of changes to one of the edge cases in get_contours so that the test would pass for the 3D case. I was getting a pathological contour where the location of the corner vertex that was added depended on which neighboring vertex was considered (more likely to happen for a small test image), so I decided to average the first and last points instead of just using the last one. And I also added the corner vertex at both the start and end of the list, to be consistent with the other cases.

ethanbb commented 4 months ago

Oops, I should probably change it to take None as the default for slice_dim instead of a string - didn't think of it.

ethanbb commented 4 months ago

Also, I didn't change it because I wasn't sure if it's technically a bug, but I noticed that while find_contours limits the max coordinate to d1-1 and d2-1, if a corner is added, it is (d2, d1) (so outside the image area).

pgunn commented 4 months ago

I'll give this some testing monday or tuesday or next week and if things look good I'll merge. The changes look good.