- 
                Notifications
    
You must be signed in to change notification settings  - Fork 26
 
Legacy Isotropic Whitening #1223
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
Conversation
| 
           Cleanup and changed some random camel casing towards our more typical var names (bgRadius->bg_radius, batchSize->batch_size). Need to finish the GPU interop and begin testing.  | 
    
          Codecov ReportAttention: Patch coverage is  
 Additional details and impacted files@@             Coverage Diff             @@
##           develop    #1223      +/-   ##
===========================================
- Coverage    90.63%   90.43%   -0.21%     
===========================================
  Files          132      132              
  Lines        13705    13939     +234     
===========================================
+ Hits         12422    12606     +184     
- Misses        1283     1333      +50     ☔ View full report in Codecov by Sentry.  | 
    
ba8690a    to
    be97013      
    Compare
  
    | 
           Initial tests of  I will start some larger runs in the background. This just needs some self review and should be ready for further consideration.  | 
    
ada6f23    to
    ad5bade      
    Compare
  
    ad5bade    to
    5603d6c      
    Compare
  
    need gwindow
[skip ci]
some issues [skip ci]
5603d6c    to
    d7a73f3      
    Compare
  
    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.
This looks great. Just a couple small things.
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!
| 
           
  | 
    
* batch legacy_whiten 2d * fft opts legacy_whitening * profile and implement Yoels validdist opt
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.
Nice work! Some questions and remarks (mostly naming) here.
| 
           I think I addressed the suggestions. This also makes the normalization optional. I will begin retesting this against MATLAB and with the full size recon.  | 
    
| 
           Reproduced the test case in the notebook. Experimental data recon will finish tomorrow, but I don't anticipate any surprises given the test batches were good.  | 
    
| 
           Experimental recon was also good.  | 
    
| 
           Great! Just one name suggestion and we should be good to go.  | 
    
This PR implements the legacy pre-whitening code from MATLAB.
Also includes two other cleanups. First, some of the
Filtermethods (_create_filter) are simplified/reconciled and enforced withabc. Second, theImageAccessorimage count attribute is changed fromnum_imageston_images. This allows anImageAccessorto be passed in place of anImagestack in the static methods (epsdR,epsdS) I've ported in this PR, but is otherwise mostly an internal change.