Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

List of shifts for pw_rigid motion correction is difficult to interpret #310

Closed
ethanbb opened this issue Jul 24, 2024 · 3 comments · Fixed by #317
Closed

List of shifts for pw_rigid motion correction is difficult to interpret #310

ethanbb opened this issue Jul 24, 2024 · 3 comments · Fixed by #317

Comments

@ethanbb
Copy link
Collaborator

ethanbb commented Jul 24, 2024

Just want to bring up this part of the code in get_shifts:

if pw_rigid:
n_pts = shifts.shape[1]
n_lines = shifts.shape[2]
xs = [np.linspace(0, n_pts, n_pts)]
ys = []
for i in range(shifts.shape[0]):
for j in range(n_lines):
ys.append(shifts[i, :, j])

My understanding is that shifts has a shape of (n_dims, n_frames, n_patches). This code flattens that to a list of length n_dims * n_patches of 1D arrays of length n_frames. Is there a good reason for this? For consistency with the rigid case, imo it would make more sense to keep the output as a list of length n_dims where each entry is an n_frames x n_patches matrix (or the transpose).

If z shifts are also added to the output (which I just added to #300), this change will make it much easier to pull out shifts along a particular dimension (without necessarily knowing whether mcorr was done in 2D or 3D).

@kushalkolar
Copy link
Collaborator

Sure, welcome to go ahead and modify as you feel appropriate! @ArjunPutcha wrote this years ago and I don't think either of us can remember why it's like this.

@ethanbb
Copy link
Collaborator Author

ethanbb commented Sep 8, 2024

I'm cautious about breaking the current API, but is there any reason to keep the first output as it is currently?

xs = [np.linspace(0, n_pts, n_pts)]

This is confusing on several points:

  • n_pts (i.e. number of frames) is already one of the dimensions of the other output, so it's redundant
  • It's a 1-element list of an ndarray for some reason
  • There's an off-by-1 error in the use of linspace, so the elements aren't integers

@kushalkolar
Copy link
Collaborator

I think usage of this method is quite rare so I think breaking the API here is fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants