ComputationalCryoEM / ASPIRE-Python

Algorithms for Single Particle Reconstruction
http://spr.math.princeton.edu
GNU General Public License v3.0
46 stars 21 forks source link

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

Open garrettwrong opened 2 weeks ago

garrettwrong commented 2 weeks ago

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.

Relates to #995 , #676

garrettwrong commented 1 week ago

Updates. I've had some luck (got a recon) taking the MATLAB shifts and negating them along with flipping XY.

I've also been able to get a recon following merging similar patches in #1180 .

Josh has found that our Simulation default offsets generate poor estimated shifts in MATLAB, but offsets based on more realistic values appear to work in both packages (although it seems the python code with 1180 patches measures better, nice).

Im getting the feeling our default Simulation offsets are too aggressive for our purposes, and should probably be reduced to better reflect the experimental datasets we intend to reconstruct. Based on the datasets I got from Yoel, offsets uniform in xy <10% would be easy to implement and a better initial default configuration (for testing purposes).

garrettwrong commented 5 days ago

It looks like we can also address #966 along with these.

janden commented 1 day ago

Makes sense to tighten the default distribution of the shifts. What do we have currently?

garrettwrong commented 1 day ago

The default is Normal(0,1) * L/16 in both X and Y. With this we tend to be getting extremal shifts. Could clip it, change var, or just use a uniform...