Skip to content

estimate-shifts appear to have X-Y flipped #1176

@garrettwrong

Description

@garrettwrong

Did a quick look over estimate shifts code in commonline base and ran some sanity tests.

If I take the current unit test for estimating shifts and replace the non-zero shift case (currently skipped, 😆 ) with a set of shifts that are all offsets of [1,2], the code returns values roughly near [2,1]. Interesting. They are not anywhere near close enough for allclose, but using np.round() indicated that we're in the right area already after flipping XY. Flipping XY can be accomplished by changing the these lines. (or by [:,::-1] the resulting shift array directly). Perhaps there is a deeper reason, but I didn't go any farther at this time.

Using these XY flipped estimated shifts with a small example pipeline (having small shifts), I think I can see some improvement.

I found the indexing arithmetic quite burdensome to read. I'm thinking it should be cleaned up and in the process, compared with the MATLAB code by taking an set of Rij rotation matrices from one of these tests (or pipeline demo).

Discussed with @j-c-c and main tasks below. These may make sense to come as separate PRs.

  • Figure out this flipping issue
  • Refactor the array indexing so it is clearer
  • Compare with MATLAB (ideally reproduce, or minimally understand where and why we diverge)
  • Create a functional unit test for the base class that actually runs.
  • Discuss where/how to call estimate_shifts (automatically in oriented source?)
  • Check this "memory factor" code (ideally, have way we can just disable it, we have hi-mem....)
  • Update docs/demos to utilize the shifts

Relates to #995 , #676

Metadata

Metadata

Labels

bugSomething isn't workingcleanup

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions