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

Correctly set imageset_id column in integration #1762

Merged
merged 9 commits into from
Jul 2, 2021

Conversation

jbeilstenedmands
Copy link
Contributor

Currently, the imageset_id column is only output by dials.integrate if include_bad_reference=True (which we turn on by default for xia2). Fortunately the result is accidentally correct for integrating a single-sweep (due to the default null value of flex.int being 0) but wrong for multi sweep data.

The fix is to actually set the imageset_id at the prediction stage of dials.integrate.

To reproduce the bug, take any multi-sweep data and import together in dials.import

dials.import sweep1*.cbf sweep2*.cbf
dials.find_spots imported.expt
dials.index imported.expt strong.refl
dials.integrate indexed.* include_bad_reference = True

Now if you open the integrated files in the dials.image_viewer, currently all integrated reflections will be shown on the first imageset, and any unintegrated reflections are shown correctly on their imagesets. If include_bad_reference=False (default for dials.integrate) then the image viewer correctly works out which reflections belong to which image through the id column.

command_line/integrate.py Outdated Show resolved Hide resolved
command_line/integrate.py Outdated Show resolved Hide resolved
command_line/integrate.py Outdated Show resolved Hide resolved
tests/command_line/test_integrate.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 23, 2021

Codecov Report

Merging #1762 (c7083a0) into main (0008175) will increase coverage by 0.13%.
The diff coverage is 78.46%.

❗ Current head c7083a0 differs from pull request most recent head 68f8ebc. Consider uploading reports for the commit 68f8ebc to get more accurate results

@@            Coverage Diff             @@
##             main    #1762      +/-   ##
==========================================
+ Coverage   67.14%   67.28%   +0.13%     
==========================================
  Files         615      616       +1     
  Lines       69053    69061       +8     
  Branches     9616     9623       +7     
==========================================
+ Hits        46365    46467     +102     
+ Misses      20756    20652     -104     
- Partials     1932     1942      +10     

Copy link
Member

@Anthchirp Anthchirp left a comment

Choose a reason for hiding this comment

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

cf. #1764

tests/command_line/test_integrate.py Outdated Show resolved Hide resolved
tests/command_line/test_integrate.py Outdated Show resolved Hide resolved
tests/command_line/test_integrate.py Outdated Show resolved Hide resolved
jbeilstenedmands and others added 2 commits June 25, 2021 14:00
Co-authored-by: Markus Gerstel <2102431+Anthchirp@users.noreply.github.com>
@ndevenish
Copy link
Member

This appears to not update any unintegrated reflections - is this information just permanently lost, at the moment?

@ndevenish
Copy link
Member

After discussion, we concluded that this is fine and I misinterpreted the wider context of the code.

@ndevenish ndevenish merged commit cc37e55 into main Jul 2, 2021
@ndevenish ndevenish deleted the imageset_id_in_integration branch July 2, 2021 13:38
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

4 participants