- 
                Notifications
    
You must be signed in to change notification settings  - Fork 26
 
Relion >= 3.1 offsets convention: _rlnOriginX(Y)Angst #1302
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
          Codecov Report❌ Patch coverage is  
 Additional details and impacted files@@             Coverage Diff             @@
##           develop    #1302      +/-   ##
===========================================
+ Coverage    90.61%   90.65%   +0.04%     
===========================================
  Files          133      133              
  Lines        14389    14433      +44     
===========================================
+ Hits         13038    13084      +46     
+ Misses        1351     1349       -2     ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
  | 
    
| 
           What's the update here? Thanks  | 
    
          
 The update is I felt very unsatisfied with this implementation. It changes the behavior of our ImageSource classes and involved adapting a lot of tests. I've opened a PR (#1317) that maintains current behavior of our ImageSource classes and just updates the metadata convention for offsets. Going to leave both PRs until we have a chance to discuss which is preferred.  | 
    
a7af69f    to
    e0687f5      
    Compare
  
    b279d25    to
    0182f7c      
    Compare
  
    | 
           Cool. This is still a fair amount of changes, but a lot of it is test code, so I think we can get it in this release if it can be made ready soon. Thanks  | 
    
… fix pixel_size bug. Fix broken tests.
…el_size for CoordinateSource/ArrayImageSource. Fix downsample offsets.
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.
Sorry, almost there, one issue, otherwise minor tweaks and this'll be good enough to push forward. This source stuff gets dicey fast.
Thanks!
| rln_src_32.offsets, sim.offsets, rtol=0, atol=atol, strict=True | ||
| ) | ||
| np.testing.assert_allclose(rln_src_64.offsets, sim.offsets, rtol=0, atol=atol) | ||
| np.testing.assert_allclose(rln_src_32.offsets, sim.offsets, rtol=0, atol=atol) | 
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.
cool thanks
…oherent between source and metadata.
| 
           @garrettwrong Ok, sorry that took until end of day. Here you go.  | 
    
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.
Great, thanks!
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.
Sorry for the delay on this.
Looks great! Just a few things.
| centered_fft=self.centered_fft, | ||
| ) | ||
| 
               | 
          ||
| return Image(im_ds, pixel_size=im.pixel_size).stack_reshape( | 
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.
So we are bypassing the standard Image.downsample method in order to keep the pixel_size the same. Why?
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.
By the time this generation pipeline is kicked off by requesting images the pixel_size has already been adjusted as an image attribute and in metadata in the ImageSource.downsample method. If we use Image.downsample here the pixel size will get adjusted twice.
        
          
                src/aspire/utils/misc.py
              
                Outdated
          
        
      | if not close: | ||
| warnings.warn( | ||
| f"User provided pixel_size: {reference_pixel_size} angstrom, does not match" | ||
| f" pixel_size found in metadata: {pixel_sizes} angstrom. Setting" | 
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 is a little confusing, since I don't think we always check the return value and act on it. Should this be updated?
Adopt Relion's newer offsets metadata fields: #1118
_rlnOriginX(Y)Angstin favor of_rlnOriginX(Y)