From b35672a0c34d769b6ed74413dc8487513464347e Mon Sep 17 00:00:00 2001 From: leonarski_f Date: Sat, 22 Aug 2026 22:20:41 +0200 Subject: [PATCH] Read any rotation axis by name, and tell a stationary axis from a sweep Two things the goniometer handling conflated. The axis name is free-form everywhere that writes it - the API imposes only minLength, the CBOR map uses the name as its key, and tests/CBORTest.cpp round trips one literally called "z" - but the reader looked for exactly "/entry/sample/transformations/omega". A sweep recorded as "phi" therefore came back as stills, in the viewer and in rugnux, with nothing to indicate it. The reader now walks the transformations group and takes whichever axis is a rotation, preferring one that turns; the grid scan is read independently rather than as the else-branch of the same test, since a grid scan can be taken at a given head position. Second: "an axis is defined" and "the axis is turning" were the same question, answered inconsistently - GetImagesPerFile checked the increment, IsRotationIndexing did not, and the CBOR decoder deleted zero-increment axes outright so the ambiguity could never surface. GoniometerAxis::IsScanning now asks it explicitly and the call sites go through it, so a stationary axis can be carried without being mistaken for rotation data. That mistake is not hypothetical: RotationIndexerCounter leaves its stride at zero for a zero increment, and Process() then never fires, so indexing would silently never run. Keeping stationary axes is also what lets the writer state where the head was for a still or a grid scan, which is the next step. JFJochReader_Goniometer_NonOmegaName covers the naming case through the writer and back; nothing did before, because both existing round trips use "omega". Co-Authored-By: Claude Opus 5 (1M context) --- common/DiffractionExperiment.cpp | 7 ++- common/GoniometerAxis.cpp | 4 ++ common/GoniometerAxis.h | 4 ++ docs/CHANGELOG.md | 2 + frame_serialize/CBORStream2Deserializer.cpp | 13 +++-- reader/HDF5MetadataSource.cpp | 58 ++++++++++++++------- rugnux/rugnux_cli.cpp | 5 +- tests/JFJochReaderTest.cpp | 50 ++++++++++++++++++ 8 files changed, 115 insertions(+), 28 deletions(-) diff --git a/common/DiffractionExperiment.cpp b/common/DiffractionExperiment.cpp index bb671b18..43639719 100644 --- a/common/DiffractionExperiment.cpp +++ b/common/DiffractionExperiment.cpp @@ -1137,7 +1137,7 @@ int64_t DiffractionExperiment::GetImagesPerFile() const { // is not a problem - past the limit the writer parallelism is worth more, one file going to one // writer (see ZMQStream2Pusher::SendImage). const auto goniometer = GetGoniometer(); - if (goniometer.has_value() && (goniometer->GetIncrement_deg() != 0.0f) + if (goniometer.has_value() && goniometer->IsScanning() && (image_num > 0) && (image_num <= ROTATION_SINGLE_FILE_IMAGE_LIMIT)) return image_num; @@ -1763,7 +1763,10 @@ int64_t DiffractionExperiment::GetDarkMaskNumberOfFrames() const { } bool DiffractionExperiment::IsRotationIndexing() const { - return GetGoniometer().has_value() && indexing.GetRotationIndexing(); + // A stationary axis is not rotation data, however it got here. Without this, a constant axis + // reaches RotationIndexerCounter, whose stride stays zero, and indexing then never runs at all. + const auto goniometer = GetGoniometer(); + return goniometer.has_value() && goniometer->IsScanning() && indexing.GetRotationIndexing(); } DiffractionExperiment &DiffractionExperiment::RunNumber(uint64_t input) { diff --git a/common/GoniometerAxis.cpp b/common/GoniometerAxis.cpp index bd56d392..dc4b0f8e 100644 --- a/common/GoniometerAxis.cpp +++ b/common/GoniometerAxis.cpp @@ -66,6 +66,10 @@ float GoniometerAxis::GetIncrement_deg() const { return increment; } +bool GoniometerAxis::IsScanning() const { + return increment != 0.0f; +} + Coord GoniometerAxis::GetAxis() const { return axis; } diff --git a/common/GoniometerAxis.h b/common/GoniometerAxis.h index 6c3fd097..ba382a39 100644 --- a/common/GoniometerAxis.h +++ b/common/GoniometerAxis.h @@ -28,6 +28,10 @@ public: [[nodiscard]] std::string GetName() const; [[nodiscard]] float GetStart_deg() const; [[nodiscard]] float GetIncrement_deg() const; + // Whether the axis actually turns during the acquisition. An axis can be present and stationary + // - a grid scan or a still taken at a particular head position - which is a different question + // from whether one is defined at all, and the two used to be conflated. + [[nodiscard]] bool IsScanning() const; [[nodiscard]] std::optional GetScreeningWedge() const; [[nodiscard]] float GetWedge_deg() const; [[nodiscard]] float GetAngle_deg(float image_number) const; diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 63aca881..1fc6a14e 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. +* 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. **Breaking change to OpenAPI** - regenerate the client (`jfjoch-client` 1.0.0-rc.162, `frontend/src/client`): diff --git a/frame_serialize/CBORStream2Deserializer.cpp b/frame_serialize/CBORStream2Deserializer.cpp index d62460a7..692e9b6e 100644 --- a/frame_serialize/CBORStream2Deserializer.cpp +++ b/frame_serialize/CBORStream2Deserializer.cpp @@ -969,13 +969,12 @@ namespace { } cborErr(cbor_value_leave_container(&value, &map_value)); - if (increment == 0) - return {}; - else { - auto g = GoniometerAxis(name, start, increment, axis, helical); - g.ScreeningWedge(screening_wedge); - return g; - } + // A stationary axis (increment 0) is kept: it says where the head was, which is real + // information for a still or a grid scan. Whether the data is a rotation sweep is asked + // separately, through GoniometerAxis::IsScanning. + auto g = GoniometerAxis(name, start, increment, axis, helical); + g.ScreeningWedge(screening_wedge); + return g; } void ProcessROIConfig(StartMessage &message, const nlohmann::json &j) { diff --git a/reader/HDF5MetadataSource.cpp b/reader/HDF5MetadataSource.cpp index 5d40edb5..6f5e234a 100644 --- a/reader/HDF5MetadataSource.cpp +++ b/reader/HDF5MetadataSource.cpp @@ -604,21 +604,40 @@ HDF5MetadataSource::OpenResult HDF5MetadataSource::Open(const std::string &filen metadata.SourceName(master_file->GetString("/entry/source/name")); dataset->experiment.ImportInstrumentMetadata(metadata); + // The rotation axis is whatever the file calls it. The name is free-form throughout the API, + // the CBOR stream and the writer, so looking only for "omega" - as this did - read a sweep + // recorded as "phi" back as stills, silently. Prefer an axis that actually turns; fall back + // to a stationary one, which still says where the head was. if (master_file->Exists("/entry/sample/transformations")) { - if (master_file->Exists("/entry/sample/transformations/omega")) { - auto omega = ReadAxis(master_file.get(), "omega"); - dataset->experiment.Goniometer(omega); - } else if (master_file->Exists("/entry/sample/grid_scan")) { - GridScanSettings grid( - master_file->GetInt("/entry/sample/grid_scan/n_fast"), - master_file->GetFloat("/entry/sample/grid_scan/step_x") * 1e6f, - master_file->GetFloat("/entry/sample/grid_scan/step_y") * 1e6f, - master_file->GetOptBool("/entry/sample/grid_scan/snake_scan").value_or(false), - master_file->GetOptBool("/entry/sample/grid_scan/vertical_scan").value_or(false) - ); - grid.ImageNum(number_of_images); - dataset->experiment.GridScan(grid); + std::optional stationary; + for (const auto &name: master_file->FindLeafs("/entry/sample/transformations")) { + auto axis = ReadAxis(master_file.get(), name); + if (!axis.has_value()) + continue; + if (axis->IsScanning()) { + dataset->experiment.Goniometer(axis); + stationary.reset(); + break; + } + if (!stationary.has_value()) + stationary = axis; } + if (stationary.has_value()) + dataset->experiment.Goniometer(stationary); + } + + // Independent of the axis: a grid scan can be taken at a given head position, so the two are + // not alternatives. + if (master_file->Exists("/entry/sample/grid_scan")) { + GridScanSettings grid( + master_file->GetInt("/entry/sample/grid_scan/n_fast"), + master_file->GetFloat("/entry/sample/grid_scan/step_x") * 1e6f, + master_file->GetFloat("/entry/sample/grid_scan/step_y") * 1e6f, + master_file->GetOptBool("/entry/sample/grid_scan/snake_scan").value_or(false), + master_file->GetOptBool("/entry/sample/grid_scan/vertical_scan").value_or(false) + ); + grid.ImageNum(number_of_images); + dataset->experiment.GridScan(grid); } auto tmp = master_file->ReadOptVector("/entry/sample/unit_cell"); @@ -1060,16 +1079,19 @@ std::optional HDF5MetadataSource::ReadAxis(HDF5Object *file, con std::vector angle; dataset.ReadVector(angle); - if (angle.size() < 2) + if (angle.empty()) + return {}; + + if (dataset.ReadAttrStr("transformation_type") != "rotation") return {}; std::vector end = file->ReadOptVector(dname + "_end"); + // A single value, or every value the same, is a stationary axis: it says where the head was + // rather than that anything turned. Increment 0 is the honest description of that, and + // GoniometerAxis::IsScanning is what separates it from a sweep. double start = angle[0]; - double incr = angle[1] - angle[0]; - - if (dataset.ReadAttrStr("transformation_type") != "rotation") - return {}; + double incr = (angle.size() < 2) ? 0.0 : angle[1] - angle[0]; std::vector axis_vec = dataset.ReadAttrVec("vector"); if (axis_vec.size() != 3) diff --git a/rugnux/rugnux_cli.cpp b/rugnux/rugnux_cli.cpp index efa2989a..09af6a62 100644 --- a/rugnux/rugnux_cli.cpp +++ b/rugnux/rugnux_cli.cpp @@ -1809,7 +1809,10 @@ static int RunRugnux(int argc, char **argv) { // (two-pass indexing) by default; --force-still forces per-frame stills. The rotation flags // (-R / --single-pass-rotation / --force-rotation-lattice) still request rotation explicitly and // choose the pass/lattice; at this point they show up as rotation_indexing already being set. - const bool has_goniometer = experiment.GetGoniometer().has_value(); + // A stationary axis records where the head was, not that the crystal turned - processing such a + // dataset as rotation would leave the indexer with a zero stride and never index anything. + const bool has_goniometer = experiment.GetGoniometer().has_value() + && experiment.GetGoniometer()->IsScanning(); if (force_still) { if (rotation_indexing) { logger.Error("--force-still conflicts with -R / --single-pass-rotation / --force-rotation-lattice"); diff --git a/tests/JFJochReaderTest.cpp b/tests/JFJochReaderTest.cpp index c3b5db93..82629ca2 100644 --- a/tests/JFJochReaderTest.cpp +++ b/tests/JFJochReaderTest.cpp @@ -395,6 +395,56 @@ TEST_CASE("JFJochReader_Goniometer", "[HDF5][Full]") { REQUIRE(H5Fget_obj_count(H5F_OBJ_ALL, H5F_OBJ_ALL) == 0); } +// The axis name is free-form in the API, on the wire and in the writer - tests/CBORTest.cpp round +// trips one literally called "z". The reader used to look only for "omega", so a sweep recorded +// under any other name came back as stills, with nothing to indicate it. This is that case. +TEST_CASE("JFJochReader_Goniometer_NonOmegaName", "[HDF5][Full]") { + DiffractionExperiment x(DetJF(1)); + + x.FilePrefix("test17b").ImagesPerTrigger(950).OverwriteExistingFiles(true); + x.BeamX_pxl(100).BeamY_pxl(200).DetectorDistance_mm(150) + .IncidentEnergy_keV(WVL_1A_IN_KEV).PixelSigned(false).BitDepthImage(16) + .FrameTime(std::chrono::microseconds(500), std::chrono::microseconds(10)); + x.Goniometer(GoniometerAxis("phi", 12, 0.2f, Coord(-1,0,0),{})); + + RegisterHDF5Filter(); + + std::vector image(x.GetPixelsNum(), 0); + { + StartMessage start_message; + x.FillMessage(start_message); + + FileWriter file_set(start_message); + + DataMessage message{}; + for (int i = 0; i < 5; i++) { + message.image = CompressedImage(image, x.GetXPixelsNum(), x.GetYPixelsNum()); + message.number = i; + REQUIRE_NOTHROW(file_set.WriteHDF5(message)); + } + + EndMessage end_message; + end_message.max_image_number = 5; + file_set.WriteHDF5(end_message); + file_set.Finalize(); + } + { + JFJochHDF5Reader reader; + reader.ReadFile("test17b_master.h5"); + auto dataset = reader.GetDataset(); + + REQUIRE(dataset->experiment.GetGoniometer().has_value()); + CHECK(dataset->experiment.GetGoniometer()->GetName() == "phi"); + CHECK(dataset->experiment.GetGoniometer()->GetStart_deg() == 12.0); + CHECK(dataset->experiment.GetGoniometer()->GetIncrement_deg() == Catch::Approx(0.2f).margin(0.00001f)); + CHECK(dataset->experiment.GetGoniometer()->IsScanning()); + CHECK(dataset->experiment.GetGoniometer()->GetAxis().x == -1); + } + remove("test17b_master.h5"); + + REQUIRE(H5Fget_obj_count(H5F_OBJ_ALL, H5F_OBJ_ALL) == 0); +} + TEST_CASE("JFJochReader_GridScan", "[HDF5][Full]") { DiffractionExperiment x(DetJF(1));