Bug #970
closedwrong cs for makestack2 phase flip by eman
100%
Description
I just noticed that the when phase flipping by stack using eman, makestack2 is using 2 instead of the cs recorded in the database. This can have a major effect on all aspects of reconstruction.
Updated by Amber Herold over 13 years ago
- Target version set to Appion/Leginon 2.1.0
Updated by Amber Herold over 13 years ago
- Assignee set to Scott Stagg
Scott, can you provide more details on the bug so that we can reproduce it?
The CS value is obtained from the database using the CTF estimation run. Are you CTF correcting the images prior to making the stack and specifying the phase-flipping option? If so, then makestack2 will fail, because it gets the CS value from the CTF run?
What is the exact error that you're getting? Can you send the attached logfile / jobfile? It'd be good to know where the CS value needs to be set, and whether to provide it as a default / option in the python.
Updated by Scott Stagg over 13 years ago
It is not an execution error, it is something that nobody thought about. When makestack was first written, Cs was hard coded as 2.0. You can see that in line 442 of makestack2 in the current trunk version (also wrong on lines 416 and 466) that it is hard coded as 2
parmstr = ("parm=%f,200,1,%.3f,0,17.4,9,1.53,%i,2,%f" %(defocus, ampconst, voltage, apix))
The 2 right before the last %f is the cs value. What this means is that any appion user who uses eman to phaseflip and who is not using a microscope with Cs of 2 is having their phases scrambled at high resolution.
Updated by Amber Herold over 13 years ago
- Status changed from New to Assigned
- Assignee changed from Scott Stagg to Amber Herold
OK. I see now. I'm thinking I'll use the same method to get the cs value that is used for phaseFlipAceTwo() on line 484. Did you already make a fix for it? Or just change the value for your installation?
Also, there are many other hard-coded values in that parameter string and I could not find documentation on what they are. Do you (or Neil/Gabe/Anchi) know? It would be nice to clean it up a bit and at least comment what the values are.
Updated by Neil Voss over 13 years ago
I went back to the original makestack 1 commit r4438 before I joined the lab. It had a CS of 2 hard coded in:
cmd="applyctf %s %s parm=%f,200,1,0.1,0,17,0.4,9,1.53,%i,2,%f setparm flipphase" %(input,output,params["df"],params["kv"],params["apix"])
The program is documented here,
http://blake.bcm.tmc.edu/emanwiki/EMAN1/Programs/ApplyCtf
I do not think I ever knew what the parameters are and still don't.
Updated by Amber Herold over 13 years ago
Yes, looked at that documentation today, but the parameters do not seem to match what appears in the function. I don't see CS.
Updated by Scott Stagg over 13 years ago
If you were to launch ctfit, the parameters in applyctf go in order of the ones that go down the left side of ctfit. I think I confirmed this by opening a stack file in python with the EMAN libraries. The other hard coded numbers have to do with the envelope and noise parameters. They are ignored when only doing a phaseflip.
Amber, I have not coded anything that can be committed. I just changed the number locally. I haven't updated in a while, and I'm worried about breaking stuff if I do update.
Updated by Amber Herold over 13 years ago
- Target version changed from Appion/Leginon 2.1.0 to Appion/Leginon 2.1.1
wont be able to get this done for this release, will add to branch asap.
Updated by Neil Voss over 13 years ago
- Status changed from Assigned to In Code Review
- % Done changed from 0 to 100
Hey Amber, I fixed the bug in the trunk (r14941), but I am not really setup to test it. Could you have someone make sure it is working correctly?
Updated by Gabriel Lander over 13 years ago
- Assignee changed from Amber Herold to Gabriel Lander
I tested it, but it's still using a value of 2.0 instead of 2.6 here, let me make sure it's not a database issue at this end.
Updated by Amber Herold over 13 years ago
Thanks Neil. Yeah, I can test it here. Since release was delayed, maybe I can get it in this afternoon. But would like to hear back from Gabe first as well re his DB...
Updated by Gabriel Lander over 13 years ago
- Status changed from In Code Review to In Test
- Assignee changed from Gabriel Lander to Amber Herold
Since we only updated our database a couple weeks ago, the majority of our "ApCtfData" rows for all our projects have an "None" value for "cs". I added code to check the ctf estimation parameters in the case that this column is empty.
Should work now.
r14942
Updated by Amber Herold over 13 years ago
Nice work guys. I may still do some cleanup after the release. I'd like to pull out the common tidbits into a separate function rather than repeating the code 3 times. I'll check into the branch now. Gabe, were you able to test it or do I need to do another run here?
Updated by Gabriel Lander over 13 years ago
It's tested and works fine at this end.
Updated by Neil Voss over 13 years ago
It was originally common code, but had to be split because the way the ctf values are determined and applied are quite different, but we could go back to a common for only the applyctf part. In one the ctf is different for each particle, but applied to a stack of particle, another we are applying the same ctf to a stack of particles, and third we are applying the ctf to a single, large image.
Makestack has so many features, granted we use them all, but man the code is hard to follow.
Updated by Amber Herold over 13 years ago
I'm removing:
print bestctfvalue sys.exit()
at line 538 in makestack2.py...it was sneaky and made it's way into r14942.
Updated by Amber Herold over 13 years ago
- Status changed from In Test to Closed