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

Fixes for EMBL beamlines at PETRA #626

Merged
merged 5 commits into from
Jul 31, 2023

Conversation

christianbecke
Copy link

Processing data recently collected at PETRA P13 using DIALS/XDS failed because the rotation axis was defined incorrectly. There is a new Eiger CdTe detector installed at P14 with serial number E-32-0129 and the Eiger with serial number E-32-0107 was moved to P13. This lead to the P14 rotation axis settings being applied to the P13 data, which resulted in indexing failures.

@codecov

This comment was marked as resolved.

@graeme-winter
Copy link
Collaborator

Started to have a quick look at this now -

  • do you have a couple of example images you could send me so I can be sure that I agree that everything is correct (I am sure it is but I would like to check)
  • do you mind if I take the liberty of adding a newsfragment once we have the changes correct
  • you are also at liberty to add yourself to the AUTHORS file if you would like, since you are in the most literal sense making yourself an author by contributing code

I will have a read through the change set and make some in-line comments

Copy link
Collaborator

@graeme-winter graeme-winter left a comment

Choose a reason for hiding this comment

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

Small change set, but that is probably in line with what is needed. If the code for P13 and P14 is effectively the same I would keep only one of them and have it capture the serial numbers for both - unless you anticipate some future specialisation which will need different implementations.

Not yet approved as we are lacking a news fragment and I would like to check that the code makes sense by running it with real data.

src/dxtbx/format/FormatCBFMiniEigerPetraP13.py Outdated Show resolved Hide resolved
src/dxtbx/format/FormatCBFMiniEigerPetraP13.py Outdated Show resolved Hide resolved
@ndevenish
Copy link
Collaborator

Ah, so if I'm understanding this properly, P13 now works on the plain FormatCBFMiniEiger, so doesn't need a special format class any more.

However, presumably this will break old data being analysed with the detector when it was on the previous beamtime; do you have a changeover date that we can use as a threshold for checking the serial number on the P14 Format class?

The E-32-0017 Eiger moved to P13 on 2021-5-22, where the goniometer axis no
longer needs overriding.

Check for that date in the MiniCBF, and check for the serial matching the date
range.
@ndevenish
Copy link
Collaborator

So, I think that this covers the fix and move between beamlines. Thanks to Gleb Bourenkov, we know that the E-32-0107 Eiger moved to P13 on May 22nd 2021. After this date, the standard MiniCBF handling works fine, so we only need to handle P14.

This should now work out what serial is expected on P14 for the timestamp, and checks for that serial.

My only worry is that we don't have any example data files to test this with.

@graeme-winter
Copy link
Collaborator

@ndevenish anything stopping this from being merged?

@ndevenish ndevenish merged commit 64bcb32 into cctbx:main Jul 31, 2023
11 checks passed
toastisme pushed a commit to toastisme/dxtbx that referenced this pull request Jul 18, 2024
The E-32-0017 Eiger moved to P13 on 2021-5-22, where the goniometer axis no
longer needs overriding.

Check for that date in the MiniCBF, and check for the serial matching the date
range.

Co-authored-by: Christian Becke <christian.becke@fu-berlin.de>
Co-authored-by: Nicholas Devenish <ndevenish@gmail.com>
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.

4 participants