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

Reindex refls_for_sym to reference setting in dials.symmetry #2183

Merged
merged 5 commits into from
Aug 10, 2022
Merged

Reindex refls_for_sym to reference setting in dials.symmetry #2183

merged 5 commits into from
Aug 10, 2022

Conversation

kmdalton
Copy link
Contributor

@kmdalton kmdalton commented Aug 1, 2022

In #2042, @dagewa shared a data set which should index as P 1 21 1. However, dials.symmetry reports P 1 2 1. Looking into this, I realized that the underlying cause is that the reflection table passed into the systematic absence search has not been reindexed to the canonical setting of this Laue group. This leads to dials.symmetry searching the wrong crystallographic basis for absent reflections. I believe this PR fixes the bug, but I need some more familiar eyes on the problem.

I don't think I have the ability to assign reviewers. Can @jbeilstenedmands and perhaps @rjgildea have a look?

Currently in dials.symmetry, if the Laue group is
set to Auto, and the supplied data are not in the
canonical setting for a given space group, they
are not reindexed before being passed to
`run_systematic_absences_checks`. This leads to
a failure in identifying the appropriate screw
axes. This commit adds an additional call to
`_reindex_experiments_reflections` which should
fix this (bug).
Copy link
Contributor

@jbeilstenedmands jbeilstenedmands left a comment

Choose a reason for hiding this comment

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

Thanks for spotting and fixing this. I had a look through the symmetry code and I don't think anything else needs changing for the other code paths, so this looks good to be merged in.

@rjgildea
Copy link
Contributor

rjgildea commented Aug 2, 2022

Thanks for this bug fix - I've managed to reproduce the bug (and demonstrate the fix) using the "x4wide" dataset in dials.data:

$ dials.symmetry $(dials.data get -q x4wide_processed)/AUTOMATIC_DEFAULT_scaled.{expt,refl}
...
Analysing systematic absences

Laue group: P 4/m m m
Performing systematic absence checks on scaled data
Read 112069 predicted reflections
Selected 66080 scaled reflections
Removed 12079 reflections with d <= 1.29
Combined 25 partial reflections with other partial reflections
Removed 2534 reflections below partiality threshold
Laue group: P 4/m m m
+--------------+---------+---------------+--------------+---------------+--------------+-------------------+------------------+
| Screw axis   |   Score |   No. present |   No. absent |   <I> present |   <I> absent |   <I/sig> present |   <I/sig> absent |
|--------------+---------+---------------+--------------+---------------+--------------+-------------------+------------------|
| 41c          |       0 |             7 |           20 |       3430.34 |      308.319 |             7.632 |            3.33  |
| 21a          |       1 |            16 |           13 |       1797.27 |        1.713 |             5.585 |            0.075 |
| 42c          |       1 |            14 |           13 |       2157.25 |       -1.75  |             8.516 |            0.062 |
+--------------+---------+---------------+--------------+---------------+--------------+-------------------+------------------+
+---------------+---------+
| Space group   |   score |
|---------------+---------|
| P 4 2 2       |  0      |
| P 4 21 2      |  0      |
| P 41 2 2      |  0      |
| P 42 2 2      |  0      |
| P 41 21 2     |  0.0004 |
| P 42 21 2     |  0.9996 |
+---------------+---------+
Recommended space group: P 42 21 2
Saving reindexed experiments to symmetrized.expt in space group P 42 21 2
Saving 112069 reindexed reflections to symmetrized.refl

The correct space group for this should be P 43 21 2, however dials.symmetry has incorrectly found P 42 21 2.

After applying your bug fix, dials.symmetry does now find the correct space group (or rather, its enantiomorphic equivalent):

$ dials.symmetry $(dials.data get -q x4wide_processed)/AUTOMATIC_DEFAULT_scaled.{expt,refl}
...
Analysing systematic absences

Laue group: P 4/m m m
Performing systematic absence checks on scaled data
Read 112069 predicted reflections
Selected 66080 scaled reflections
Removed 12079 reflections with d <= 1.29
Combined 25 partial reflections with other partial reflections
Removed 2534 reflections below partiality threshold
Laue group: P 4/m m m
+--------------+---------+---------------+--------------+---------------+--------------+-------------------+------------------+
| Screw axis   |   Score |   No. present |   No. absent |   <I> present |   <I> absent |   <I/sig> present |   <I/sig> absent |
|--------------+---------+---------------+--------------+---------------+--------------+-------------------+------------------|
| 41c          |   1     |             4 |           12 |       326.529 |        3.31  |             5.855 |            0.149 |
| 21a          |   1     |            16 |           15 |      1890.19  |       -0.869 |             9.455 |            0.066 |
| 42c          |   0.997 |             9 |            7 |       147.976 |        2.007 |             2.728 |            0.094 |
+--------------+---------+---------------+--------------+---------------+--------------+-------------------+------------------+
+---------------+---------+
| Space group   |   score |
|---------------+---------|
| P 4 2 2       |       0 |
| P 4 21 2      |       0 |
| P 41 2 2      |       0 |
| P 42 2 2      |       0 |
| P 41 21 2     |       1 |
| P 42 21 2     |       0 |
+---------------+---------+
Recommended space group: P 41 21 2
Space group with equivalent score (enantiomorphic pair): P 43 21 2
Saving reindexed experiments to symmetrized.expt in space group P 41 21 2
Saving 112069 reindexed reflections to symmetrized.refl

I'll look into adding a test for this bug fix.

@kmdalton
Copy link
Contributor Author

kmdalton commented Aug 2, 2022

It seemed like a bug to me. Glad you all concur. Props to @dagewa for sharing that P 21 dataset. It was super helpful.

@jbeilstenedmands
Copy link
Contributor

@rjgildea It looks like changing procrunnner to subprocess caused issues for the windows test job, so I have reverted that commit. It would be great to get this PR in now, and I noticed we haven't started the procrunner -> subprocess conversion in dials tests (only xia2), so probably best to leave that for now and do that as a separate PR on the whole codebase another time.

@jbeilstenedmands jbeilstenedmands merged commit 4925678 into dials:main Aug 10, 2022
ndevenish pushed a commit that referenced this pull request Sep 14, 2022
Currently in dials.symmetry, if the Laue group is
set to Auto, and the supplied data are not in the
canonical setting for a given space group, they
are not reindexed before being passed to
`run_systematic_absences_checks`. This leads to
a failure in identifying the appropriate screw
axes. This commit adds an additional call to
`_reindex_experiments_reflections` which should
fix this (bug).

Test bugfix on x4wide dataset

Co-authored-by: Kevin Dalton <kmdalton@pop-os.localdomain>
Co-authored-by: Richard Gildea <rjgildea@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants