Skip to content
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

Calculate image size limits correctly for data access in dials.rs_mapper #2364

Merged
merged 5 commits into from
Mar 14, 2023

Conversation

dagewa
Copy link
Member

@dagewa dagewa commented Mar 9, 2023

This was spotted during testing of #2362 with I23 data. The program would segfault, which was eventually tracked down to the indices used to access image data being reversed from expected given their accessor.

This was only noticed now, because I23 panels are highly elongated, with size (195, 2463). For previous uses with squarish single panel images it is likely that the pixels within the specified resolution limit (default 6 Å) had indices that were valid for either x or y.

The resultant output is clearly quite different. Here is a comparison of the resultant maps in Coot for the dials-data insulin dataset (dials.data get insulin), with the map from this branch in purple and that from main in green

image

It is not merely a case of transposing one to the other, because the pixel is also rotated to a particular position around an axis with fixed orientation wrt to the image.

This may have consequences for dials.export format=json.

I think this might need more testing to understand the issue before it is merged.

@dagewa
Copy link
Member Author

dagewa commented Mar 9, 2023

The latest change makes maps that align very closely. I need to try this on the multi-panel branch to see if it works for I23
image

@dagewa
Copy link
Member Author

dagewa commented Mar 9, 2023

Merging this into the multi-panel-rs-mapper shows that this works without crashing for I23, and the results look right.

image

My current understanding is that the problem was actually in the limits passed into get_target_pixels, not the image access in fill_voxels. The changes here fix that and change some variable names to make it clearer.

@dagewa dagewa marked this pull request as ready for review March 9, 2023 15:53
@dagewa dagewa changed the title Swap indices for image data access in dials.rs_mapper Calculate image size limits correctly for data access in dials.rs_mapper Mar 9, 2023
Comment on lines -191 to +192
xlim, ylim = imageset.get_raw_data(0)[0].all()
nfast, nslow = panel.get_image_size()

Copy link
Member Author

Choose a reason for hiding this comment

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

To avoid any further confusion, this is the critical change. The rest is just cosmetic.

@codecov
Copy link

codecov bot commented Mar 9, 2023

Codecov Report

Merging #2364 (a0bc0b5) into main (cfe100e) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2364      +/-   ##
==========================================
- Coverage   82.90%   82.83%   -0.07%     
==========================================
  Files         593      593              
  Lines       68592    68592              
  Branches     9221     9221              
==========================================
- Hits        56863    56817      -46     
- Misses       9613     9658      +45     
- Partials     2116     2117       +1     

@dagewa dagewa merged commit 945479f into main Mar 14, 2023
@dagewa dagewa deleted the rs_mapper-indices-swap branch March 14, 2023 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants