Skip to content

Conversation

@garrettwrong
Copy link
Collaborator

@garrettwrong garrettwrong commented Feb 3, 2021

Closes #379

WIP, just pushing current state now that appears to reasonably reproduce the original ctf branch. I had some minor trouble validating the results, but I think have a good handle on it.

Outstanding:

  • Unit Test (call class)
  • Command line doc
  • Tinker with the interior point to try and yield better determinism (this would rm the repro option I added)
  • Convert pyFFTW to leverage ASPIRE fft implementations.
  • Convert eigen decomp to ASPIRE implementation
  • Decide how to implement the dtype (performance is better in singles on my machine).
  • Docstring details (probably bug A once the other items are satisfied)
  • A final scientific validation (presumably A)

@garrettwrong garrettwrong added the enhancement New feature or request label Feb 3, 2021
@garrettwrong garrettwrong self-assigned this Feb 3, 2021
@garrettwrong
Copy link
Collaborator Author

@ayeltg , I've updated ctf so it can go into ASPIRE's current develop branch. Would you be able to:

  • Fill in any details in the doc strings that would be relevant to a scientific user
  • Check if this version of the code is reproducing satisfactory scientific outputs

I have checked with example mrc file(s), and there is some discussion about the level of determinism. in #379 , but I am consistent with ctf up to run-run variability for my test file. I presume you have better data sets to check against, and also can interpret the correctness of the results.

Please let me know what you think, thanks.

@garrettwrong
Copy link
Collaborator Author

Notes from meeting with Ayelet (2/16):

  • Output values in the context of their physical interpretations seem good. Defocus U and V are in nano meters which makes the run to run variation sub Angstrom. Angle variation small fraction of a degree. So the run to run variation seems acceptable.
  • Ayelet can help with completing the remaining doc strings.
  • The sample.mrc file used for (unit) testing is reasonable. Ayelet will run with a reference Matlab code and other CTF estimation tools for an independent comparison.

The above should round out the draft.

@garrettwrong
Copy link
Collaborator Author

I squashed some commits from ayeltg and rebased on top of latest develop (since this was going to be a force push anyway).

As I understand the last thing to check is a comparison with a third party tool. Then I will read over the changes again before undrafting for Joakim's review.

@codecov
Copy link

codecov bot commented Mar 16, 2021

Codecov Report

Merging #380 (04b91a4) into develop (004ba5e) will increase coverage by 0.56%.
The diff coverage is 93.28%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #380      +/-   ##
===========================================
+ Coverage    86.93%   87.50%   +0.56%     
===========================================
  Files           91       95       +4     
  Lines         6171     6553     +382     
===========================================
+ Hits          5365     5734     +369     
- Misses         806      819      +13     
Impacted Files Coverage Δ
src/aspire/commands/_config.py 0.00% <0.00%> (ø)
src/aspire/commands/estimate_ctf.py 0.00% <0.00%> (ø)
src/aspire/operators/__init__.py 100.00% <ø> (ø)
src/aspire/utils/coor_trans.py 96.11% <66.66%> (+7.76%) ⬆️
src/aspire/ctf/ctf_estimator.py 98.87% <98.87%> (ø)
src/aspire/ctf/__init__.py 100.00% <100.00%> (ø)
src/aspire/image/image.py 91.05% <100.00%> (+2.22%) ⬆️
src/aspire/utils/__init__.py 100.00% <100.00%> (ø)
src/aspire/utils/misc.py 97.36% <100.00%> (+0.14%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 004ba5e...04b91a4. Read the comment docs.

@garrettwrong
Copy link
Collaborator Author

I've been corresponding with Ayelet by email and am happy to report that after a minor patch the results of her comparisons are that we have good scientific correspondence between ASPIRE-Python the third party ctffind and Aspire-MATLAB.

Two future improvements include a better interface to run aspire-ctf over an entire dataset, and GPU acceleration (maybe next hackathon). Will create feature request issues.

There is one difference that came out. It appears that the MATLAB version may be transposed (so angles related by 90* offset). If I understood correctly the Python version is currently matching third party. We are going to review this before undrafting, but up to this point, I think that concludes the draft of this feature.

@garrettwrong garrettwrong marked this pull request as ready for review March 30, 2021 18:26
@garrettwrong garrettwrong requested a review from janden May 10, 2021 14:02
@garrettwrong
Copy link
Collaborator Author

I think I've hit all the conversations. There are two we should get feedback from Ayelet on (the backround_p1 loop, and the circular vs elliptical averaging.

We have a few issues open to follow up on this code, but I think they are scoped for more success at later dates. This was a bit much at once.

@janden
Copy link
Collaborator

janden commented May 17, 2021

Ok. Everything looks good for me otherwise.

@ayeltg Would you be able to take a look at the two remaining conversations?

@garrettwrong
Copy link
Collaborator Author

We haven't heard from Ayelet so I am moving to merge this now.

Any follow ups can be smaller fresh issues (maybe that will make it easier to contribute). The ones most concerning have been made into issues related to this PR through Github.

Thanks everyone.

@garrettwrong garrettwrong merged commit 95dff05 into develop May 26, 2021
@garrettwrong garrettwrong deleted the ctf_rebased branch June 8, 2021 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants