Skip to content

Conversation

@garrettwrong
Copy link
Collaborator

Looks like we had some errors in our CTFFilter formula. Because this is mainly tested tautologically, multiple errors slipped by.

To confirm error wrt MATLAB and test I performed the following:

  • Created a simulation data set with CTF in Python
  • Saved the dataset (as a STARfile)
  • Read the STARfile in MATLAB
  • Ran MATLABs phaseflip
  • Loaded the MATLAB result mrcs back into Python
  • Compared Python phase_flip with the MATLAB result
  • Compared loading the STAR file in Python then phase_flipping and compare with MATLAB result

I am considering adding the hard coded reference test of CTFFilter.evaluate_grid (say at 5px) so this is less likely to happen again. While I prefer less hard coded tests, clearly testing our formula against our own formula is not enough 😇 . Can discuss other options at our next dev meeting.

@garrettwrong garrettwrong added bug Something isn't working cleanup labels Jan 7, 2025
@garrettwrong garrettwrong self-assigned this Jan 7, 2025
@garrettwrong garrettwrong changed the title Ctf formula2 Correct CTFFilter formula/units Jan 7, 2025
@codecov
Copy link

codecov bot commented Jan 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.66%. Comparing base (a29670b) to head (a890725).
Report is 2 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1217   +/-   ##
========================================
  Coverage    90.66%   90.66%           
========================================
  Files          132      132           
  Lines        13706    13707    +1     
========================================
+ Hits         12427    12428    +1     
  Misses        1279     1279           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@j-c-c j-c-c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@garrettwrong garrettwrong marked this pull request as ready for review January 8, 2025 20:59
@garrettwrong garrettwrong requested a review from janden as a code owner January 8, 2025 20:59
@garrettwrong
Copy link
Collaborator Author

Merging patch.

@garrettwrong garrettwrong merged commit a94bf0d into develop Jan 9, 2025
35 checks passed
@garrettwrong garrettwrong deleted the ctf_formula2 branch January 9, 2025 15:37
@janden
Copy link
Collaborator

janden commented Jan 15, 2025

I am considering adding the hard coded reference test of CTFFilter.evaluate_grid (say at 5px) so this is less likely to happen again. While I prefer less hard coded tests, clearly testing our formula against our own formula is not enough 😇 . Can discuss other options at our next dev meeting.

I think this is fine. Another reference we might want to consider is RELION. We can use relion_project to generate a projection (with and without CTF). This won't be as tight of course (considering projection is done slightly differently), but I'd consider it closer to a gold standard than Matlab ASPIRE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working cleanup

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants