cctbx / cctbx_project

Computational Crystallography Toolbox
https://cci.lbl.gov/docs/cctbx
Other
223 stars 119 forks source link

Rotation axis inverted for SPring8 EIGER files #282

Closed rjgildea closed 5 years ago

rjgildea commented 5 years ago

The rotation axis is currently inverted for the SPring8 CCP4 workshop 2018 example data. This appears to be a regression between 1.11 and 1.12:

$ for i in 10 11 12; do echo DIALS 1.$i: ;  dials.show 1.$i.json | grep Rotation; done
DIALS 1.10:
    Rotation axis:   {-1,0,0}
DIALS 1.11:
    Rotation axis:   {-1,0,0}
DIALS 1.12:
    Rotation axis:   {1,0,0}

I have added an xfailing test in the dls-eiger-nexus branch: https://github.com/cctbx/cctbx_project/blob/dls-eiger-nexus/dxtbx/tests/format/test_FormatEigerNearlyNexus.py#L40

Files on disk at DLS @ /dls/mx-scratch/rjgildea/zenodo/spring8-ccp4-2018/1443110/ccp4school2018_bl41xu/05/data03/data03_master.h5

rjgildea commented 5 years ago

I have narrowed it down to a regression between 1.12.1 and 1.12.2:

$ for i in 0 1 2 3; do echo DIALS 1.12.$i: ;  dials.show 1.12.$i.json | grep Rotation; done
DIALS 1.12.0:
    Rotation axis:   {-1,0,0}
DIALS 1.12.1:
    Rotation axis:   {-1,0,0}
DIALS 1.12.2:
    Rotation axis:   {1,0,0}
DIALS 1.12.3:
    Rotation axis:   {1,0,0}
rjgildea commented 5 years ago

I suspect it is related to this change from @graeme-winter: https://github.com/cctbx/cctbx_project/commit/8ce75c61a41049f59c58900f7bd7316707604598#diff-9d62d7c0dec42965c0a5f836e8240f37

graeme-winter commented 5 years ago

P(this) ~ 1.0

keitaroyam commented 5 years ago

@rjgildea Are you going to introduce SPring-8 specific code? EIGER master files at SPring-8 are already modified so that 'vector' attribute of omega points to -1,0,0 (reversed phi). This modification was not right?

graeme-winter commented 5 years ago

There will have to be SPring8 specific code - the rotation axis is -1,0,0 in the imgCIF frame but in the NeXus / McStas frame this is actually 1,0,0 - but coordinate frames are literally all over the place with the various installations around the world.

I am sure that a lot of applications already check the serial number to work out the correct interpretation of the axis - this will be another instance of this. Would be great to have this completely understood and a universal system used but this seems to be close to impossible in real life...

rjgildea commented 5 years ago

I have added a specific format class FormatHDF5EigerNearlyNexusSPring8 that inverts the rotation axis: https://github.com/cctbx/cctbx_project/commit/75faa8f291a89bd3c63ec2b8dc87d6c5aa0192f8

rjgildea commented 5 years ago

R.e. NeXus/McStas coordinate system: http://download.nexusformat.org/sphinx/design.html#the-nexus-coordinate-system

rjgildea commented 5 years ago

However, it does say:

This choice is arbitrary and any other choice should be possible as long as it is used consistently and application code that reads NeXus files does not assume any prior knowledge of the chosen coordinate system.

keitaroyam commented 5 years ago

Sorry I still do not understand why master.h5 information is not enough to define geometry.. If I recall correctly we followed the Dectris documentation where an incoming beam direction was fixed probably, but it had a sign error and we needed to flip some axis..

Anyway if SPring-8 specific code will be used, please note that there are currently three EIGER detectors at SPring-8: E-18-0103: BL32XU, EIGER X 9M E-32-0114: BL41XU, EIGER X 16M E-32-0112: BL44XU, EIGER X 16M

graeme-winter commented 5 years ago

🤔 I can see there is some confusion / ambiguity here

My understanding was that the NeXus specification took as a default the McStas coordinate system & there is code out there which uses this assumption - our NeXus representative confirmed this when we were putting things together.

What DECTRIS do / have done with the headers is in DIALS understood as "nearly NeXus" i.e. it is largely inspired by the NeXus format but that was work in progress when the Eigers started using it. The DIALS implementation of this "nearly NeXus" format also makes a lot of assumptions which work in most places though - clearly - not quite at SPring8 -

It's a moderately confusing situation

I think a practical solution here is to have specific format instances which cover the three instruments you list and work from there. Probably as a community we are overdue visiting (and critically discussing) the HDF5 file structures we are using across different facilities...

rjgildea commented 5 years ago

@graeme-winter are these lines correct? Might be the root of the issue?

https://github.com/cctbx/cctbx_project/blob/0cb683ada6ebd3823ae3275dfd5e866f0663c789/dxtbx/format/FormatHDFEigerNearlyNexus.py#L148-L151

https://github.com/cctbx/cctbx_project/blob/0cb683ada6ebd3823ae3275dfd5e866f0663c789/dxtbx/format/FormatHDFEigerNearlyNexus.py#L199-L200

phyy-nx commented 5 years ago

Hi all, those lines come from 2017 research and communication with Dectris. See: https://github.com/cctbx/cctbx_project/pull/30

See also my email from May of 2017, NeXus, Eiger, IUCr, and the inverted world, which I've copied below.

At the time, Andreas did say there was going to be a fix issued for the Eiger that would correct their implementation to match the NeXus standard. Perhaps that has gone live? Thanks, -Aaron

Hi Keitaro, could you try branch dials-324-coord-fixes? Hopefully this gives equivalently correct answers (positive anomalous peaks and right-handed alpha helices). Internally, this branch reverts the workaround from a few weeks ago that inverted the beam vector, fast, slow and origin vectors. Instead it applies what I think is a set of basis shifts that should put us on the IUCr coordinate system like the rest of dxtbx.

Here are waaaaay more details for those interested. It looks like there were two root causes that caused things to be incorrect. 1) There is a sign flip in the Eiger metadata, specifically the detector distance, which makes it not match the documentation. 2) We weren't converting between the NeXus and IUCr coordinate systems in nexus.py properly. These two problems together can be worked around by inverting everything, but a) this puts us in a coordinate system inconsistent with the rest of dxtbx, including an inverted beam vector which makes me nervous, and b) doesn't work for NeXus data that isn't from the Eiger, like the CSPAD CBF NeXus stuff I'm working on right now.

This is the invert everything commit: https://github.com/cctbx/cctbx_project/commit/ede9683a5d0511b5f378850851db0600dc2bfdfb

This is my commit in the dials-324-coord-fixes branch reverting the invert everything commit and adding my patches: https://github.com/cctbx/cctbx_project/commit/97be73724901f0179bca0f95c28301f6e8078888

Evidence for 1)

First, here's the relevant documentation: Eiger coordinate system: https://www.dectris.com/nexus.html?file=tl_files/tl_files_restricted/download/header_docs/DectrisGeometryDocumentation.pdf NeXus coordinate system: http://download.nexusformat.org/sphinx/design.html#nexus-coordinate-systems

These systems match each other: Z is along the beam, pointing from source to sample (IE detector distance is positive), Y is against gravity and X completes a right handed coordinate system. Now, compare to what is actually recorded in Dectris metadata:

$ cat show_vectors.py import sys, h5py

handle = h5py.File(sys.argv[1], 'r') fast_axis = handle['/entry/instrument/detector/geometry/orientation/value'][0:3] slow_axis = handle['/entry/instrument/detector/geometry/orientation/value'][3:6] detector_offset_vector = handle['/entry/instrument/detector/geometry/translation/distances'][()] print "Fast vector", fast_axis print "Slow vector", slow_axis print "Origin vector", detector_offset_vector

$ libtbx.python show_vectors.py ./eiger16MNov2015/2015_11_10/insu6_1_master.h5 Fast vector [-1. 0. 0.] Slow vector [ 0. -1. 0.] Origin vector [ 0.14384994 0.163352 -0.160019 ]

First, note that the X and Y components of the origin vector, as well as the fast and slow vectors, are compatible with the detector origin at the upper left of the detector, given the Eiger/NeXus coordinate system. If we display the image with the image viewer, this is what we expect. However, also note the Z value of the origin vector is negative, which would put the detector behind the sample given the Eiger/NeXus coordinate system. Therefore, we need to swap the sign of Z in NearlyNexus when reading Eiger data to match the documented Eiger/NeXus coordinate system.

Evidence for 2) Referring back to the NeXus coordinate system documentation: http://download.nexusformat.org/sphinx/design.html#nexus-coordinate-systems

See the note: The NeXus definition of +z is opposite to that in the IUCr International Tables for Crystallography, volume G, and consequently, +x is also reversed.

Here are the IUCr docs to verify: http://www.iucr.org/__data/iucr/cifdic_html/2/cif_img.dic/Caxis.html

Z is along the beam, pointing from sample to source (IE detector distance is negative), Y is against gravity and X completes a right handed coordinate system. So, if we swap the signs of X and Z in nexus.py when reading axes from NeXus, we're good.

Putting it all together:

Print header using live code that inverts the world (edited for brevity): $ dxtbx.print_header eiger16MNov2015/2015_11_10/insu6_1_master.h5 === .eiger16MNov2015/2015_11_10/insu6_1_master.h5 === Using header reader: FormatEigerNearlyNexus Beam: wavelength: 1 sample to source direction : {0,0,-1}

Detector: Panel: fast_axis: {1,0,0} slow_axis: {0,1,0} origin: {-143.85,-163.352,160.019}

Using patches in branch dials-324-coord-fixes $ dxtbx.print_header eiger16MNov2015/2015_11_10/insu6_1_master.h5 === .eiger16MNov2015/2015_11_10/insu6_1_master.h5 === Using header reader: FormatEigerNearlyNexus Beam: wavelength: 1 sample to source direction : {0,0,1}

Detector: Panel: fast_axis: {1,0,0} slow_axis: {0,-1,0} origin: {-143.85,163.352,-160.019}

Incidentally, these two sets of vectors are superimposable without a mirror operator: spin the whole experiment around the X axis 180 degrees. Nicely shows they are equivalent for measuring reciprocal space coordinates.

Whew!

On Tue, Jan 22, 2019 at 9:23 AM Richard Gildea notifications@github.com wrote:

@graeme-winter https://github.com/graeme-winter are these lines correct? Might be the root of the issue?

https://github.com/cctbx/cctbx_project/blob/0cb683ada6ebd3823ae3275dfd5e866f0663c789/dxtbx/format/FormatHDFEigerNearlyNexus.py#L148-L151

https://github.com/cctbx/cctbx_project/blob/0cb683ada6ebd3823ae3275dfd5e866f0663c789/dxtbx/format/FormatHDFEigerNearlyNexus.py#L199-L200

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/cctbx/cctbx_project/issues/282#issuecomment-456486043, or mute the thread https://github.com/notifications/unsubscribe-auth/AM2OyrL7irzlVfdTmMLiIwZB2EFeC6KXks5vF0kRgaJpZM4aHmDa .