- 
                Notifications
    
You must be signed in to change notification settings  - Fork 26
 
Preprocess updates #1314
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Preprocess updates #1314
Conversation
          Codecov Report❌ Patch coverage is  
 Additional details and impacted files@@             Coverage Diff             @@
##           develop    #1314      +/-   ##
===========================================
- Coverage    90.61%   90.60%   -0.01%     
===========================================
  Files          133      133              
  Lines        14373    14394      +21     
===========================================
+ Hits         13024    13042      +18     
- Misses        1349     1352       +3     ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
  | 
    
daf3608    to
    e0052ec      
    Compare
  
    | 
           Currently testing the JSB gallery updates for this merged with the mctf work. If it seems okay I'll move this pr onto review.  | 
    
| 
           For 10028 80S dataset loaded as single precision and processed to 129 pixels in ASPIRE-Python these changes achieve max nrmse 7.6313734e-07 and avg nrmse 3.7122902e-07 across all images as compared to the MATLAB cryo_workflow_preprocess utilizing the expected sequence (as I understand it). Note, that comparison included merging in the tentative CTF changes proposed in #1315 . Besides the different defaults I also had to alter our code to upcast some (most) of the internal processing computations to double precision. This is essentially what the MATLAB code does in general, casting to singles at the end via the mrc save. Some computations test well for small/single image cases but differences show on larger images/stacks. I expect there will be discussion about how to take the changes, but we can at least replicate the preprocessed dataset to my satisfaction now.  | 
    
| 
           Alright @j-c-c , this should be ready to review. Then I can get to the CTF stuff that is holding up your other PRs.  | 
    
          
 Sounds good. I'll start reviewing!  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just one small docstring fix and two typos while you're in there.
        
          
                src/aspire/image/xform.py
              
                Outdated
          
        
      | """ | ||
| Initialize Xform to crop Image to a specific size. | ||
| :param L: int - new size, should be <= the current size | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the should be <= the current size in there by mistake? If we're padding wouldn't L > current size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea I forgot to delete this after I extended from just crop to crop_pad. removed. thanks
        
          
                tests/test_preprocess_pipeline.py
              
                Outdated
          
        
      | np.testing.assert_allclose(pad_odd_to_even_many.images[:], ref) | ||
| 
               | 
          ||
| # Padding odd to odd by many should pad the first and last cols equally. | ||
| # This test will also excecise `fill_value` | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not important, but "exercise".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed comment typo
        
          
                tests/test_preprocess_pipeline.py
              
                Outdated
          
        
      | np.testing.assert_allclose(pad_even_to_even_many.images[:], ref) | ||
| 
               | 
          ||
| # Padding even to odd by many should pad the first and last+1 cols equally. | ||
| # This test will also excecise `fill_value` | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed comment typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for going through all this stuff. Just some minor formulation stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Resolves #1310 #1311 #1312
I put this together hastily so I'll want to self-review and consider/discuss the changes more carefully before going farther...