-
Notifications
You must be signed in to change notification settings - Fork 46
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
In profile-fitted integration, don't integrate reflections with a high fraction of invalid pixels. #1640
Conversation
…than a given fraction of the foreground pixels are valid. This is controlled by a parameter valid_foreground_threshold (range 0->1), default value 0.6. For #1638
Codecov Report
@@ Coverage Diff @@
## main #1640 +/- ##
=======================================
Coverage 66.63% 66.63%
=======================================
Files 616 616
Lines 68949 68951 +2
Branches 9601 9595 -6
=======================================
+ Hits 45943 45945 +2
Misses 21070 21070
Partials 1936 1936 |
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.
Stage 0 review of looking at the code changes - seem sane - propose moving the default value to a unique place i.e. the master phil, not scattering 0.6 everywhere, and there seemed to be an unrelated change in the mix.
I will run some actual integration tests now (no pun intended) to assess the impact of these changes. I will approve once that change is made.
I think this is a large fanfare bug fix which warrants a 3.4.2 @ndevenish
working_directory=tmpdir, | ||
) | ||
assert not result.returncode and not result.stderr | ||
table = flex.reflection_table.from_file(tmpdir / "integrated.refl") |
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.
Propose stronger test would be to check that there are no reflection centroids in tile join regions?
Tested on the x4-wide dataset. This PR gives significant improvement in merging R-factors, Isigma compared to current release (dials-3.4), with 10K fewer reflections. Compared to dials-3.3, R-factors are slightly worse however there are 6K more reflections. This PR, with threshold value of 0.75:
dials-3.4 (after #1297):
dials-3.3.5 (before #1297), processed to same resolution:
|
Updated the default threshold to be 0.75, to be more conservative, and this is also the value for the equivalent idea in XDS. |
Functional review: in hindsight this problem is not as egregious as at first thought but the fix is very worthwhile and justifies a little fanfair. Example data set / comparable images / before and after: With patches in this PR: As may be expected modest but worthwhile improvements in the quality of the merged data as recorded by the merging stats, no evidence of a negative impact -> thank you green light me |
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.
As above, change set looks good to go thank you
Agree with setting MIN_PK equivalent to 0.75
…h fraction of invalid pixels. (#1640) For profile-fitted integration, only integrate reflections that have a high fraction of valid foreground pixels. Controlled by parameter valid_foreground_threshold (range 0 -> 1). * Add test to display broken profile-fitting behaviour. * Screen out reflections for integration by profile-fitting where less than a given fraction of the foreground pixels are valid. * Fix bug in integration terminal-report output * Make threshold value 0.75 to be more conservative. * Update further default values * Add debug log statement for number of reflections filtered out
Features -------- - ``dials.cosym``: Significantly faster via improved computation of functional, gradients and curvatures (#1639) - ``dials.integrate``: Added parameter ``valid_foreground_threshold=``, to require a minimum fraction of valid pixels before profile fitting is attempted (#1640) Bugfixes -------- - ``dials.cosym``: Cache cases where Rij is undefined, rather than recalculating each time. This can have significant performance benefits when handling large numbers of sparse data sets. (#1634) - ``dials.cosym``: Fix factor of 2 error when calculating target weights (#1635) - ``dials.cosym``: Fix broken ``engine=scipy`` option (#1636) - ``dials.integrate``: Reject reflections with a high number of invalid pixels, which were being integrated since 3.4.0. This restores better merging statistics, and prevents many reflections being incorrect profiled as zero-intensity. (#1640)
Features -------- - ``dials.cosym``: Significantly faster via improved computation of functional, gradients and curvatures (#1639) - ``dials.integrate``: Added parameter ``valid_foreground_threshold=``, to require a minimum fraction of valid pixels before profile fitting is attempted (#1640) Bugfixes -------- - ``dials.cosym``: Cache cases where Rij is undefined, rather than recalculating each time. This can have significant performance benefits when handling large numbers of sparse data sets. (#1634) - ``dials.cosym``: Fix factor of 2 error when calculating target weights (#1635) - ``dials.cosym``: Fix broken ``engine=scipy`` option (#1636) - ``dials.integrate``: Reject reflections with a high number of invalid pixels, which were being integrated since 3.4.0. This restores better merging statistics, and prevents many reflections being incorrect profiled as zero-intensity. (#1640)
Features -------- - ``dials.cosym``: Significantly faster via improved computation of functional, gradients and curvatures (#1639) - ``dials.integrate``: Added parameter ``valid_foreground_threshold=``, to require a minimum fraction of valid pixels before profile fitting is attempted (#1640) Bugfixes -------- - ``dials.cosym``: Cache cases where Rij is undefined, rather than recalculating each time. This can have significant performance benefits when handling large numbers of sparse data sets. (#1634) - ``dials.cosym``: Fix factor of 2 error when calculating target weights (#1635) - ``dials.cosym``: Fix broken ``engine=scipy`` option (#1636) - ``dials.integrate``: Reject reflections with a high number of invalid pixels, which were being integrated since 3.4.0. This restores better merging statistics, and prevents many reflections being incorrect profiled as zero-intensity. (#1640) - Fix rare crash in symmetry calculations when no resolution limit could be calculated (#1641)
For profile-fitted integration, only integrate reflections that have a high fraction of valid foreground pixels (currently set to
0.60.75). Controlled by parameter valid_foreground_threshold (range 0 -> 1). For #1638.This is intended so that reflections which have centroids in the panel gaps are not integrated by profile fitting. In my opinion the default parameter should be at least 0.5 so that the peak centre is approximately observed. Perhaps a higher threshold such as 0.75 is more appropriate, I plan to investigate how the data quality is affected depending on the threshold value.