dials / dials

Diffraction Integration for Advanced Light Sources
https://dials.github.io
BSD 3-Clause "New" or "Revised" License
74 stars 52 forks source link

dials.symmetry fails on multi-sweep centred data set due to instability of cctbx minimum-cell reduction routine #1037

Open graeme-winter opened 4 years ago

graeme-winter commented 4 years ago
DIALS (2018) Acta Cryst. D74, 85-97. https://doi.org/10.1107/S2059798317017235
DIALS 2.0.2-g25fc2fc43-release
The following parameters have been modified:

input {
  experiments = integrated.expt
  reflections = integrated.refl
}

Detected existence of a multi-dataset reflection table 
containing 4 datasets. 
...
Incompatible unit cell: (4.80801, 12.8219, 16.5573, 73.5157, 90.0253, 79.2275)
      median unit cell: (4.80765, 12.8188, 16.5583, 106.445, 90.0189, 100.786)
graeme-winter commented 4 years ago

Processed stepwise in /dls/tmp/gw56/i19-check/raw - integrated.(refl,expt) in there will reproduce issue.

Reproduce issue on master.

Input fine:

cs03r-sc-serv-16 raw :( $ dials.show integrated.expt |grep Unit
    Unit cell: (4.8052(4), 12.8081(10), 16.5449(12), 106.458(3), 90.007(2), 100.777(2))
    Unit cell: (4.8080(4), 12.8219(10), 16.5573(12), 106.484(3), 90.025(2), 100.773(2))
    Unit cell: (4.8097(4), 12.8156(10), 16.5593(12), 106.490(3), 90.017(2), 100.804(3))
    Unit cell: (4.8073(3), 12.8224(10), 16.5604(12), 106.432(2), 90.021(2), 100.795(2))

suspect issue is it maps one to mC lattice then finds others don't match, as mapping back is ambiguous.

rjgildea commented 4 years ago

Rather unhelpfully this works just fine on mac:

$ dials.symmetry integrated.*
$ dials.show symmetrized.expt | grep cell
    Unit cell: (25.164, 4.805, 16.545, 90.000, 106.764, 90.000)
    Unit cell: (25.192, 4.808, 16.557, 90.000, 106.794, 90.000)
    Unit cell: (25.177, 4.810, 16.559, 90.000, 106.800, 90.000)
    Unit cell: (25.191, 4.807, 16.560, 90.000, 106.741, 90.000)
rjgildea commented 4 years ago

And even more unhelpfully, I can't reproduce it with my linux sources:

$ dials.symmetry /dls/tmp/gw56/i19-check/raw/integrated.{refl,expt}
$ dials.show symmetrized.expt | grep cell
    Unit cell: (25.164, 4.805, 16.545, 90.000, 106.764, 90.000)
    Unit cell: (25.192, 4.808, 16.557, 90.000, 106.794, 90.000)
    Unit cell: (25.177, 4.810, 16.559, 90.000, 106.800, 90.000)
    Unit cell: (25.191, 4.807, 16.560, 90.000, 106.741, 90.000)
rjgildea commented 4 years ago

I don't think this is a general issue, but just a particular corner case with this specific dataset caused by the lack of robustness of the cctbx minimum-cell reduction routine. In fact this case only fails if the code is compiled on RHEL6, but works as expected on RHEL7 and mac.

rjgildea commented 4 years ago

See https://github.com/cctbx/cctbx_project/issues/419

rjgildea commented 4 years ago

Fixed by https://github.com/dials/dials/commit/6dacce377f10557624fca55a5fca06bb6cdfa87b

graeme-winter commented 4 years ago

Appears to be broken on Python 3.6.7

Grey-Area dials :) [master] $ pytest --regression test/command_line/test_symmetry.py::test_change_of_basis_ops_to_minimum_cell_1037
Test session starts (platform: darwin, Python 3.6.7, pytest 4.6.4, pytest-sugar 0.9.2)
rootdir: /Users/graeme/svn/cctbx/modules/dials, inifile: pytest.ini
plugins: sugar-0.9.2, mock-2.0.0, forked-1.1.2, xdist-1.31.0, dials-data-2.0.61
collecting ... 

―――――――――――――――― test_change_of_basis_ops_to_minimum_cell_1037 ―――――――――――――――――

mocker = <pytest_mock.plugin.MockFixture object at 0x1151f77b8>

    def test_change_of_basis_ops_to_minimum_cell_1037(mocker):
        # See https://github.com/dials/dials/issues/1037

        input_ucs = [
            (
                4.805202948916906,
                12.808064769657364,
                16.544899201125446,
                106.45808502003258,
                90.0065567098825,
                100.77735674275475,
            ),
            (
                4.808011343212577,
                12.821894835790472,
                16.557339561965573,
                106.48431244651402,
                90.0252848479048,
                100.77252933676507,
            ),
            (
                4.8096632137789985,
                12.815648858527567,
                16.55931712239122,
                106.48990701341536,
                90.01703141314147,
                100.80397887485773,
            ),
            (
                4.807294085194974,
                12.822386757910516,
                16.560411742466663,
                106.43185845358086,
                90.02067929544215,
                100.79522302759383,
            ),
        ]

        # Setup the input experiments and reflection tables
        expts = ExperimentList()
        for uc in input_ucs:
            uc = uctbx.unit_cell(uc)
            sg = sgtbx.space_group_info("P1").group()
            B = scitbx.matrix.sqr(uc.fractionalization_matrix()).transpose()
            expts.append(Experiment(crystal=Crystal(B, space_group=sg, reciprocal=True)))

        # We want to spy on the return value of this function
        mocker.spy(symmetry, "unit_cells_are_similar_to")

        # Actually run the method we are testing
        cb_ops = change_of_basis_ops_to_minimum_cell(
            expts, max_delta=5, relative_length_tolerance=0.05, absolute_angle_tolerance=2
        )
>       assert symmetry.unit_cells_are_similar_to.return_value is True
E       AssertionError: assert <MagicMock name='unit_cells_are_similar_to()' id='4652289720'> is True
E        +  where <MagicMock name='unit_cells_are_similar_to()' id='4652289720'> = <function unit_cells_are_similar_to at 0x115503488>.return_value
E        +    where <function unit_cells_are_similar_to at 0x115503488> = symmetry.unit_cells_are_similar_to

test/command_line/test_symmetry.py:301: AssertionError

 test/command_line/test_symmetry.py ⨯                            100% ██████████

Results (1.49s):
       1 failed
         - test/command_line/test_symmetry.py:248 test_change_of_basis_ops_to_minimum_cell_1037
graeme-winter commented 4 years ago

Joy - this test proves to be unstable - hooray for numerical stability

Today it works - on the same machine, same build:

Grey-Area dials :) [master] $ pytest --regression test/command_line/test_symmetry.py::test_change_of_basis_ops_to_minimum_cell_1037
Test session starts (platform: darwin, Python 3.6.7, pytest 4.6.4, pytest-sugar 0.9.2)
rootdir: /Users/graeme/svn/cctbx/modules/dials, inifile: pytest.ini
plugins: sugar-0.9.2, mock-2.0.0, forked-1.1.2, xdist-1.31.0, dials-data-2.0.61
collecting ... 
 test/command_line/test_symmetry.py ✓                            100% ██████████

Results (1.42s):
       1 passed
picca commented 4 years ago

I can confirm that I have this bug on Debian unstable

$ LIBTBX_BUILD=/usr/lib/python3/dist-packages/ pytest-3 test/command_line/test_symmetry.py::test_change_of_basis_ops_to_minimum_cell_1037
================================================================================== test session starts ==================================================================================
platform linux -- Python 3.8.3rc1, pytest-4.6.9, py-1.8.1, pluggy-0.13.0
rootdir: /usr/lib/python3/dist-packages/dials, inifile: pytest.ini
plugins: requests-mock-1.7.0, mock-1.10.4
collected 1 item                                                                                                                                                                        

test/command_line/test_symmetry.py F                                                                                                                                              [100%]

======================================================================================= FAILURES ========================================================================================
_____________________________________________________________________ test_change_of_basis_ops_to_minimum_cell_1037 _____________________________________________________________________

mocker = <pytest_mock.MockFixture object at 0x7f014701de80>

    def test_change_of_basis_ops_to_minimum_cell_1037(mocker):
        # See https://github.com/dials/dials/issues/1037

        input_ucs = [
            (
                4.805202948916906,
                12.808064769657364,
                16.544899201125446,
                106.45808502003258,
                90.0065567098825,
                100.77735674275475,
            ),
            (
                4.808011343212577,
                12.821894835790472,
                16.557339561965573,
                106.48431244651402,
                90.0252848479048,
                100.77252933676507,
            ),
            (
                4.8096632137789985,
                12.815648858527567,
                16.55931712239122,
                106.48990701341536,
                90.01703141314147,
                100.80397887485773,
            ),
            (
                4.807294085194974,
                12.822386757910516,
                16.560411742466663,
                106.43185845358086,
                90.02067929544215,
                100.79522302759383,
            ),
        ]

        # Setup the input experiments and reflection tables
        expts = ExperimentList()
        for uc in input_ucs:
            uc = uctbx.unit_cell(uc)
            sg = sgtbx.space_group_info("P1").group()
            B = scitbx.matrix.sqr(uc.fractionalization_matrix()).transpose()
            expts.append(Experiment(crystal=Crystal(B, space_group=sg, reciprocal=True)))

        # We want to spy on the return value of this function
        mocker.spy(symmetry, "unit_cells_are_similar_to")

        # Actually run the method we are testing
        cb_ops = change_of_basis_ops_to_minimum_cell(
            expts, max_delta=5, relative_length_tolerance=0.05, absolute_angle_tolerance=2
        )
        import pytest_mock

        if pytest_mock.version.startswith("1."):
>           assert symmetry.unit_cells_are_similar_to.return_value is True
E           AssertionError: assert <MagicMock name='unit_cells_are_similar_to()' id='139643462811408'> is True
E            +  where <MagicMock name='unit_cells_are_similar_to()' id='139643462811408'> = <function unit_cells_are_similar_to at 0x7f0146fdb8b0>.return_value
E            +    where <function unit_cells_are_similar_to at 0x7f0146fdb8b0> = symmetry.unit_cells_are_similar_to

test/command_line/test_symmetry.py:369: AssertionError
stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. The label will be removed automatically if any activity occurs. Thank you for your contributions.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. The label will be removed automatically if any activity occurs. Thank you for your contributions.