Skip to content

Conversation

@ethanbb
Copy link
Contributor

@ethanbb ethanbb commented Oct 27, 2025

Description

This fixes what I believe is an oversight in apply_shifts_movie. My assumption is that apply_shifts_movie is meant to be applied to a movie with the same spatial dimensions as the original movie mcorr was run on (e.g., corresponding functional imaging channel). Thus, if indices were specified to restrict motion correction to a certain field of view (or downsample by setting step to >1), the same indices should be used when applying piecewise shifts to put each shift in the right place. However, that is not currently done.

Since #1444 adds the shifts_interpolate method which interpolates and extrapolates (repeating edge) from the specific X/Y location of each piecewise shift, I just used that here, adjusting the patch centers to account for the indices. If any of the indices are not slice(None), I took the simplest approach and forced the shifts_interpolate method.

Type of change

Please delete options that are not relevant.

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

Has your PR been tested?

Unfortunately no, I don't think apply_shifts_movie is currently tested. I don't have time to add a test at this moment, but I might be able to add one later before merging.

@pgunn
Copy link
Member

pgunn commented Oct 28, 2025

This seems pretty reasonable, although I haven't really thought about mixing shifts and indices together (these felt like edge cases, haha). If you have a use for it and feel it's probably right, particularly if you add a test, I'm happy to merge this.

@ethanbb
Copy link
Contributor Author

ethanbb commented Oct 28, 2025

If you have a use for it

Yeah, I am using indices to crop out features in the FOV that are not affected by brain motion (e.g., artifacts generated by the microscope always in the same place) just for motion correction, and then re-apply the shifts to the whole movie. I think this is useful functionality that should be supported...up to now I was only doing this with rigid shifts, but I recently realized that it should do the right thing for piecewise motion correction as well.

We can leave it for now and I can add a test when I have some time.

@pgunn
Copy link
Member

pgunn commented Oct 28, 2025

By leave it, do you mean do a merge of it as-is, or leave it open as a PR? I'm open to either.

@ethanbb
Copy link
Contributor Author

ethanbb commented Oct 28, 2025

Oh, I meant leave open as a PR. Or hopefully I can at least get to the point of trying the code first. You know what, let me make it a draft PR until then, I don't want to merge code that hasn't been tested at all.

@ethanbb ethanbb marked this pull request as draft October 28, 2025 19:08
@pgunn
Copy link
Member

pgunn commented Oct 28, 2025

Ah, I may have misunderstood; I thought this was something you were already using but you haven't written discrete tests for it yet.

Sure, we can leave it as a draft.

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 this pull request may close these issues.

2 participants