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

[ethanbb]

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.

• Bug fix (non-breaking change which fixes an issue)

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):

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

After change with swap_dim = False:

[pgunn]

Provided people were only doing positional argument passing before, this is okay (and there's no in-repo code that does otherwise), although people will also have to remember that because dims is thirsty, order can only be passed as a keyword.

>>> def foo(alpha, *beta, gamma):
        print(f"{alpha=}, {beta=}, {gamma=}")

>>>
>>> foo('a', 'b1', 'b2','b3','b4','g')
alpha='a', beta=('b1', 'b2', 'b3', 'b4', 'g'), gamma=None

I hope this doesn't trip anyone up.

I wonder if we might want to keep the type definition for A but expand it to be a Union of all those types.

[ethanbb]

Provided people were only doing positional argument passing before, this is okay (and there's no in-repo code that does otherwise)

Right, I doubt anyone was passing the dimensions as keywords, but just in case we could add keyword arguments for them as well (although then we'd have to deal with some edge cases if both positional and keyword dimensions are passed).

There's also no strong reason to modify the signature at all except for adding order. Do you think we should just use com(A, d1, d2, d3=None, order='F')?

I wonder if we might want to keep the type definition for A but expand it to be a Union of all those types.

Can definitely add that if you want to err on the side of annotating every argument, although I don't feel strongly about it.

[pgunn]

Just adding order would eliminate the possibility of disruption; probably better to do that.

WRT A, in this case, maybe that type definition would be so bulky that it wouldn't be worth it; it's fine to remove it. I'll add the full form back later if it gets me enough the next time I go tackling bugs with mypy.

[ethanbb]

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]

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

[ethanbb]

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]

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]

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]

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