From 7039aa4e45107a481dfb119596e847c6978c207c Mon Sep 17 00:00:00 2001 From: leonarski_f Date: Sat, 22 Aug 2026 22:25:08 +0200 Subject: [PATCH] Stop treating a goniometer axis and a grid scan as alternatives They are not alternatives: a grid is usually collected at a particular head position, so an axis and a grid describe different parts of the same setup. The exclusion was enforced independently in four places - the API converter, the CBOR serializer, the writer and the reader - and each silently dropped the grid scan when an axis was present. Nothing warned. The writer now builds one chain from the base outwards, spindle -> chi -> phi -> helical -> grid translations, instead of two branches. NXmx applies the deepest dependency first, so the sample ends up innermost, which is what it physically is: the grid stage rides on the head and the head rides on the spindle. The grid translations consequently move inside the rotation - identical to before at omega = 0, and right rather than wrong when it is not. A grid scan with no axis at all now writes a stationary omega. NXmx has no way to say "there is no rotation", and a sample chain of translations alone is not something readers accept: dxtbx raises outright on it, so every grid-scan master we have written so far cannot be opened by DIALS. Measured on a file matching the new chain: dials.import reads it. At 0 degrees the rotation is the identity whatever the axis points along, so the conventional vector carries no geometric claim - it only has to be well formed. The API change is deliberately not breaking: no field changes type or cardinality, only the prose saying the two were exclusive, and a request that set both used to lose one silently and now does not. JFJochReader_GridScan asserted the absence of a goniometer; it now asserts the axis is present and stationary, which is the contract that matters - a grid scan must not read back as a sweep. Co-Authored-By: Claude Opus 5 (1M context) --- broker/OpenAPIConvert.cpp | 4 +- broker/jfjoch_api.yaml | 4 +- docs/CBOR.md | 2 +- docs/CHANGELOG.md | 2 + frame_serialize/CBORStream2Serializer.cpp | 3 +- tests/JFJochReaderTest.cpp | 6 +- writer/HDF5NXmx.cpp | 134 ++++++++++++---------- 7 files changed, 92 insertions(+), 63 deletions(-) diff --git a/broker/OpenAPIConvert.cpp b/broker/OpenAPIConvert.cpp index cf59c923..7cbcb38e 100644 --- a/broker/OpenAPIConvert.cpp +++ b/broker/OpenAPIConvert.cpp @@ -665,9 +665,11 @@ DatasetSettings Convert(const org::openapitools::server::model::Dataset_settings ret.TotalFlux(input.getTotalFlux()); if (input.transmissionIsSet()) ret.AttenuatorTransmission(input.getTransmission()); + // Not alternatives: a grid scan is often collected at a given head position, so an axis and a + // grid can both be set. This used to drop the grid scan silently whenever an axis was present. if (input.goniometerIsSet()) ret.Goniometer(Convert(input.getGoniometer())); - else if (input.gridScanIsSet()) + if (input.gridScanIsSet()) ret.GridScan(Convert(input.getGridScan())); if (input.spaceGroupNumberIsSet()) diff --git a/broker/jfjoch_api.yaml b/broker/jfjoch_api.yaml index 4aa8c9de..5f17efac 100644 --- a/broker/jfjoch_api.yaml +++ b/broker/jfjoch_api.yaml @@ -240,7 +240,9 @@ components: default: "indigo" schemas: grid_scan: - description: Definition of a grid scan (mutually exclusive with `rotation_axis`) + description: | + Definition of a grid scan. May be combined with a goniometer axis: a grid is often collected + at a particular head position, and a stationary axis records where that was. type: object required: - n_fast diff --git a/docs/CBOR.md b/docs/CBOR.md index fa411393..7f0a98e6 100644 --- a/docs/CBOR.md +++ b/docs/CBOR.md @@ -52,7 +52,7 @@ There are minor differences at the moment: | - - axis | Array(float) | Vector for the rotation axis | | | - - helical_step | Array(float) | Translation for helical scan for 1 image \[m\] | | | - - screening_wedge | Array(float) | Wedge for screening \[deg\] (increment would correspond to difference between screening points) | | -| grid_scan | object | Grid scan definition (optional and exclusive with rotation axis) | | +| grid_scan | object | Grid scan definition (optional). May be sent together with `goniometer`: a grid is often collected at a given head position, recorded as a stationary axis (step 0) | | | - n_fast | uint64 | Number of elements along fast axis | | | - n_slow | uint64 | Number of elements along slow axis | | | - step_x_axis | float | Step along X axis, can be negative \[m\] | | diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 1fc6a14e..571bfc33 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -15,6 +15,8 @@ This is an UNSTABLE release. It includes many experimental features, as well as * rugnux: the detector geometry is also logged in XDS's convention (`ORGX`/`ORGY`, detector axis vectors, rotation axis), so it can be compared directly with an XDS refinement. * HDF5: a data file missing next to a VDS master now reads as the error-pixel marker instead of zero counts, so those frames are masked rather than silently integrated as blank. * The writer refuses a stream whose start message declares a different pixel format than its images carry, instead of writing a master that does not describe its own data. +* A grid scan and a goniometer axis are no longer alternatives - both can be set, and the grid scan is no longer silently dropped when an axis is present. +* HDF5: a grid-scan file now records the (stationary) spindle, so it can be opened by programs that require a rotation axis; DIALS refused the previous files outright. * The rotation axis is read back from HDF5 under whatever name it carries; only `omega` was recognised before, so a sweep recorded as e.g. `phi` re-opened as stills with nothing to say so. * A goniometer axis that does not turn is now kept rather than discarded, and is distinguished from a rotation sweep - it records where the head was for a still or a grid scan. * `images_per_file` is now chosen from the acquisition when it is not given: a rotation sweep of at most 20000 images goes into a single data file, a grid scan splits on whole fast-axis rows, and stills and serial keep 1000. diff --git a/frame_serialize/CBORStream2Serializer.cpp b/frame_serialize/CBORStream2Serializer.cpp index e2b8d549..ddab842c 100644 --- a/frame_serialize/CBORStream2Serializer.cpp +++ b/frame_serialize/CBORStream2Serializer.cpp @@ -688,9 +688,10 @@ void CBORStream2Serializer::SerializeSequenceStart(const StartMessage& message) CBOR_ENC(mapEncoder, "series_id", message.run_number); CBOR_ENC(mapEncoder, "fluorescence", message.fluorescence_spectrum); + // Both, when both are present - the decoder has always read the two keys independently. if (message.goniometer) CBOR_ENC_GONIOMETER_MAP(mapEncoder, "goniometer", message); - else if (message.grid_scan) + if (message.grid_scan) CBOR_ENC_GRID_SCAN(mapEncoder, "grid_scan", message.grid_scan.value()); CBOR_ENC(mapEncoder, "jungfrau_conversion_enabled", message.jungfrau_conversion_enabled); diff --git a/tests/JFJochReaderTest.cpp b/tests/JFJochReaderTest.cpp index 82629ca2..9f41085d 100644 --- a/tests/JFJochReaderTest.cpp +++ b/tests/JFJochReaderTest.cpp @@ -490,7 +490,11 @@ TEST_CASE("JFJochReader_GridScan", "[HDF5][Full]") { auto dataset = reader.GetDataset(); - REQUIRE(!dataset->experiment.GetGoniometer().has_value()); + // A grid scan carries a stationary spindle: NXmx cannot say "no rotation", and a chain of + // translations alone is not readable (dxtbx raises on it). It must not read back as a sweep. + REQUIRE(dataset->experiment.GetGoniometer().has_value()); + CHECK(!dataset->experiment.GetGoniometer()->IsScanning()); + CHECK(dataset->experiment.GetGoniometer()->GetIncrement_deg() == 0.0f); REQUIRE(dataset->experiment.GetGridScan().has_value()); CHECK(dataset->experiment.GetGridScan()->IsSnakeScan()); diff --git a/writer/HDF5NXmx.cpp b/writer/HDF5NXmx.cpp index 3c877a71..866aa7d8 100644 --- a/writer/HDF5NXmx.cpp +++ b/writer/HDF5NXmx.cpp @@ -762,60 +762,14 @@ void NXmx::Sample(const StartMessage &start, const EndMessage &end) { depends_on = "/entry/sample/transformations/phi"; }; - if ((end.max_image_number > 0) && start.goniometer) { - HDF5Group transformations(group, "transformations"); - transformations.NXClass("NXtransformations"); - hdf5_file->HardLink("/entry/sample/transformations","/entry/sample/goniometer"); + // One chain, built from the base outwards, rather than the goniometer and the grid scan being + // alternatives. NXmx applies the deepest dependency first, so writing spindle -> chi -> phi -> + // grid translations makes the sample sit at the innermost end, which is what it physically is: + // the grid stage rides on the head, and the head rides on the spindle. + const bool write_goniometer = (end.max_image_number > 0) && start.goniometer.has_value(); + const bool write_grid_scan = start.grid_scan.has_value(); - // Prefer the rotation axis refined by the offline analysis (rugnux); the broker leaves it empty - // and the user-provided goniometer axis stands. - const std::vector axis_vector = end.refined_rotation_axis - ? std::vector{end.refined_rotation_axis->x, end.refined_rotation_axis->y, - end.refined_rotation_axis->z} - : start.goniometer->GetAxisVector(); - SaveVector(transformations, start.goniometer->GetName(), - start.goniometer->GetAngleContainer(end.max_image_number))-> - Transformation("deg", depends_on, "", "", - "rotation", axis_vector, {0,0,0}, ""); - - SaveVector(transformations, start.goniometer->GetName() + "_end", - start.goniometer->GetAngleContainerEnd(end.max_image_number)) - ->Units("deg"); - - SaveScalar(transformations, start.goniometer->GetName() + "_range_average", - start.goniometer->GetIncrement_deg()) - ->Units("deg"); - SaveScalar(transformations, start.goniometer->GetName() + "_range_total", - start.goniometer->GetIncrement_deg() * end.max_image_number) - ->Units("deg"); - depends_on = "/entry/sample/transformations/" + start.goniometer->GetName(); - - write_smargon(transformations, depends_on); - - auto helical = start.goniometer->GetHelicalStep(); - if (helical.has_value()) { - SaveVector(transformations, - start.goniometer->GetName() + "_helical_x", - start.goniometer->GetXContainer_m(end.max_image_number))-> - Transformation("m", depends_on, "", "", - "translation", {1, 0, 0}, {0,0,0}, ""); - depends_on = "/entry/sample/transformations/" + start.goniometer->GetName() + "_helical_x"; - - SaveVector(transformations, - start.goniometer->GetName() + "_helical_y", - start.goniometer->GetYContainer_m(end.max_image_number))-> - Transformation("m", depends_on, "", "", - "translation", {0, 1, 0}, {0,0,0}, ""); - depends_on = "/entry/sample/transformations/" + start.goniometer->GetName() + "_helical_y"; - - SaveVector(transformations, - start.goniometer->GetName() + "_helical_z", - start.goniometer->GetZContainer_m(end.max_image_number))-> - Transformation("m", depends_on, "", "", - "translation", {0, 0, 1}, {0,0,0}, ""); - depends_on = "/entry/sample/transformations/" + start.goniometer->GetName() + "_helical_z"; - } - } else if (start.grid_scan.has_value()) { + if (write_grid_scan) { HDF5Group grid_scan_group(group, "grid_scan"); grid_scan_group.NXClass("NXcollection"); @@ -824,14 +778,80 @@ void NXmx::Sample(const StartMessage &start, const EndMessage &end) { SaveScalar(grid_scan_group, "n_fast", start.grid_scan->GetNFast()); SaveScalar(grid_scan_group, "step_x", start.grid_scan->GetGridStepX_um() * 1e-6)->Units("m"); SaveScalar(grid_scan_group, "step_y", start.grid_scan->GetGridStepY_um() * 1e-6)->Units("m"); + } + if (write_goniometer || write_grid_scan || start.smargon_position) { HDF5Group transformations(group, "transformations"); transformations.NXClass("NXtransformations"); hdf5_file->HardLink("/entry/sample/transformations","/entry/sample/goniometer"); - // The position containers hold one entry per image; they are empty when the scan - // stopped at the first image (max_image_number == 0), so only write them otherwise. - if (end.max_image_number > 0) { + if (write_goniometer) { + // Prefer the rotation axis refined by the offline analysis (rugnux); the broker leaves it empty + // and the user-provided goniometer axis stands. + const std::vector axis_vector = end.refined_rotation_axis + ? std::vector{end.refined_rotation_axis->x, end.refined_rotation_axis->y, + end.refined_rotation_axis->z} + : start.goniometer->GetAxisVector(); + SaveVector(transformations, start.goniometer->GetName(), + start.goniometer->GetAngleContainer(end.max_image_number))-> + Transformation("deg", depends_on, "", "", + "rotation", axis_vector, {0,0,0}, ""); + + SaveVector(transformations, start.goniometer->GetName() + "_end", + start.goniometer->GetAngleContainerEnd(end.max_image_number)) + ->Units("deg"); + + SaveScalar(transformations, start.goniometer->GetName() + "_range_average", + start.goniometer->GetIncrement_deg()) + ->Units("deg"); + SaveScalar(transformations, start.goniometer->GetName() + "_range_total", + start.goniometer->GetIncrement_deg() * end.max_image_number) + ->Units("deg"); + depends_on = "/entry/sample/transformations/" + start.goniometer->GetName(); + } else if (write_grid_scan) { + // No axis was given, but the sample still sits on a spindle - it simply does not turn. + // Say so: NXmx cannot express "no rotation", and a chain of translations alone is not + // something readers accept (dxtbx raises on it outright). At 0 degrees the rotation is + // the identity whatever the axis points along, so the conventional vector below carries + // no geometric claim. + SaveScalar(transformations, "omega", 0.0f)-> + Transformation("deg", depends_on, "", "", + "rotation", {-1, 0, 0}, {0,0,0}, ""); + depends_on = "/entry/sample/transformations/omega"; + } + + // Smargon chi/phi sit between the spindle and the sample. + write_smargon(transformations, depends_on); + + if (write_goniometer) { + auto helical = start.goniometer->GetHelicalStep(); + if (helical.has_value()) { + SaveVector(transformations, + start.goniometer->GetName() + "_helical_x", + start.goniometer->GetXContainer_m(end.max_image_number))-> + Transformation("m", depends_on, "", "", + "translation", {1, 0, 0}, {0,0,0}, ""); + depends_on = "/entry/sample/transformations/" + start.goniometer->GetName() + "_helical_x"; + + SaveVector(transformations, + start.goniometer->GetName() + "_helical_y", + start.goniometer->GetYContainer_m(end.max_image_number))-> + Transformation("m", depends_on, "", "", + "translation", {0, 1, 0}, {0,0,0}, ""); + depends_on = "/entry/sample/transformations/" + start.goniometer->GetName() + "_helical_y"; + + SaveVector(transformations, + start.goniometer->GetName() + "_helical_z", + start.goniometer->GetZContainer_m(end.max_image_number))-> + Transformation("m", depends_on, "", "", + "translation", {0, 0, 1}, {0,0,0}, ""); + depends_on = "/entry/sample/transformations/" + start.goniometer->GetName() + "_helical_z"; + } + } + + // The grid stage is mounted on the head, so its translations are innermost. The position + // containers hold one entry per image and are empty if the scan stopped at the first image. + if (write_grid_scan && (end.max_image_number > 0)) { SaveVector(transformations,"grid_scan_x", start.grid_scan->GetXContainer_m(end.max_image_number)) ->Transformation("m", depends_on, "", "", "translation", {1, 0, 0}, {0,0,0}, ""); @@ -842,8 +862,6 @@ void NXmx::Sample(const StartMessage &start, const EndMessage &end) { "translation", {0, 1, 0}, {0,0,0}, ""); depends_on = "/entry/sample/transformations/grid_scan_y"; } - - write_smargon(transformations, depends_on); } group.SaveScalar("depends_on", depends_on);