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

partiality_threshold=0.4 in cosym, symmetry and ttr #1470

Merged
merged 4 commits into from
Nov 2, 2020

Conversation

rjgildea
Copy link
Contributor

Set the default partiality_threshold to 0.4 in dials.cosym, dials.symmetry and dials.two_theta_refine. The previous default of 0.99 could occasionally be overzealous in rejecting reflections, especially with particularly narrow wedges. This issue was identified by errors encountered running xia2.multiplex on James Holton's "Micro-focus Data Processing Challenge" dataset.

Previous default of 0.99 can be overzealous for particularly narrow
wedges. Set to same default as dials.scale, dials.export and
dials.merge.
Expose partiality_threshold parameter on command line.
Previous default of 0.99 can be overzealous for particularly narrow
wedges.
to reproduce the previous result (and test that the program supports
setting it on the command line).
@codecov
Copy link

codecov bot commented Oct 28, 2020

Codecov Report

Merging #1470 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1470   +/-   ##
=======================================
  Coverage   65.81%   65.82%           
=======================================
  Files         614      614           
  Lines       69410    69410           
  Branches     9508     9508           
=======================================
+ Hits        45685    45691    +6     
+ Misses      21887    21884    -3     
+ Partials     1838     1835    -3     

@dagewa
Copy link
Member

dagewa commented Oct 29, 2020

For dials.two_theta_refine is the idea to use the same set of reflections as used in dials.cosym and dials.symmetry (for data sets that are scaled)? For the latter two programs, this change increases the number of reflections accepted, whereas for dials.two_theta_refine it decreases the number, but as a result essentially the same set of reflections gets used. Is that right?

@rjgildea
Copy link
Contributor Author

for dials.two_theta_refine it decreases the number

Perhaps I'm missing something, but I don't understand why this would be the case? The default partiality_threshold for dials.util.filter_reflections.filter_reflection_table is 0.99, and previously this parameter was essentially hard-coded in dials.two_theta_refine and not user-controllable:

https://github.com/dials/dials/blob/master/util/filter_reflections.py#L457

@rjgildea
Copy link
Contributor Author

With module load dials/latest:

$ dials.two_theta_refine $(dials.data get -q l_cysteine_4_sweeps_scaled)/scaled_20_25.{expt,refl} | egrep 'Removed|Summary'
Removed 20 reflections below partiality threshold
Summary statistics for 3585 observations matched to predictions:

With this PR:

$ dials.two_theta_refine $(dials.data get -q l_cysteine_4_sweeps_scaled)/scaled_20_25.{expt,refl} | egrep 'Removed|Summary'
Summary statistics for 3605 observations matched to predictions:

I.e. with this PR, fewer reflections are removed (none in this case) for being below the partiality threshold, and more reflections in total are used in refinement.

@dagewa
Copy link
Member

dagewa commented Oct 29, 2020

I see, thanks. I didn't realise anything was filtered previously in dials.two_theta_refine - I thought it operated on all the reflections it was given. I didn't notice that it called filter_reflection_table already, I just assumed that was added here. All clear 👍

Copy link
Member

@dagewa dagewa left a comment

Choose a reason for hiding this comment

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

It makes sense that these tools use the same threshold for post-scaling reflection selection. I did wonder whether it would make sense for filter_reflection_table to set a flag, but I think that can be a separate discussion.

@rjgildea rjgildea merged commit 7b36b5c into master Nov 2, 2020
@rjgildea rjgildea deleted the partiality_threshold branch November 2, 2020 12:10
ndevenish pushed a commit that referenced this pull request Nov 9, 2020
* Default partiality_threshold=0.4 in cosym and symmetry

Previous default of 0.99 can be overzealous for particularly narrow
wedges. Set to same default as dials.scale, dials.export and
dials.merge.

* Default partiality_threshold=0.4 in ttr

Expose partiality_threshold parameter on command line.
Previous default of 0.99 can be overzealous for particularly narrow
wedges.
@ndevenish ndevenish mentioned this pull request Nov 9, 2020
ndevenish added a commit that referenced this pull request Nov 9, 2020
3.2 Branch releases will now use a fixed conda environment. This release is the first to use the same versions of all dependencies as 3.2.0.

Bugfixes
--------

- ``dials.symmetry``, ``dials.cosym`` and ``dials.two_theta_refine``: Lowered default partiality_threshold from ``0.99`` to to ``0.4``. The previous default could occasionally result in too many reflections being rejected for particularly narrow wedges. (#1470)
- ``dials.stills_process`` Improve performance when using MPI by avoiding unnecessary log file writing (#1471)
- ``dials.scale``: Fix scaling statistics output of r_anom data. (#1478)
ndevenish added a commit that referenced this pull request Nov 9, 2020
3.2 Branch releases will now use a fixed conda environment. This release is the first to use the same versions of all dependencies as 3.2.0.

Bugfixes
--------

- ``dials.symmetry``, ``dials.cosym`` and ``dials.two_theta_refine``: Lowered default partiality_threshold from ``0.99`` to to ``0.4``. The previous default could occasionally result in too many reflections being rejected for particularly narrow wedges. (#1470)
- ``dials.stills_process`` Improve performance when using MPI by avoiding unnecessary log file writing (#1471)
- ``dials.scale``: Fix scaling statistics output of r_anom data. (#1478)
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

3 participants