From df9a9c2a2c8179f9dece10d57424ac972349b918 Mon Sep 17 00:00:00 2001 From: Filip Leonarski Date: Sat, 8 Aug 2026 18:18:46 +0200 Subject: [PATCH] Fix the defects found reviewing the branch before merge Image buffer: the per-image CBOR metadata headroom had been re-derived from the online reflection cap alone, which cut it from 4 MiB to 2.55 MB while the measured worst case - reflections plus the capped spot list plus the three azimuthal arrays - is 2.9 MB, so the receiver dropped the frames with the most to say. Restore it and give it a name that both the code and its guard test read: written down twice, the two had drifted and the test kept passing against the value the code had left. Spot finding: an unset low_resolution_limit means no limit at that end, as an unset high_resolution_limit already did. An optional rather than a zero sentinel, because zero is not a natural "no limit" here - every pixel lies above it, so the plain comparison masked the whole image instead of none of it, and nothing validated the zero. The API field is no longer required; a zero is folded into the unset case at the boundary, where older clients still send it, so one spelling reaches the analysis code. The FPGA takes its fixed-point ceiling instead, since ap_ufixed<16,9> wraps above 512 A and would have masked everything. image_preprocessing: check the CUDA calls on the fused decode path - the one new GPU file with none, and the path fed by bytes we did not produce. An unchecked synchronise returned the host-written sentinel as if it were a measurement, so the decode looked successful and the fallback to the host decoder never fired. rugnux: --stride no longer writes one past the end of the per-image arrays, whose count floored where the worker loop ceils, and the written process file links the images actually processed rather than the first N - each frame's picture now sits next to its own analysis. Powder calibration: the face-centred calibrants no longer list their systematically absent rings, so the distance fit starts from a reflection that exists rather than an extinct one; the triclinic calibrant covers both signs of h and k instead of a single octant, which is only valid for a diagonal metric. The test asserted the old behaviour - one ring formula for every cubic standard - and is rewritten. CBOR: skip an unknown tagged value in the end block, as the other four blocks already do. One advance lands on the tagged item rather than past it, so an older reader fed a newer end message threw and never finalized its file. Viewer: a settings value the setter rejects no longer escapes as an uncaught throw from a worker slot, and the field offers only what the setter accepts. Space-group search: judge stage B on the same "present" cut stage A already computes. Merged sigma is floored so no reflection reads above ISa, so on a low-ISa merge the fixed cut left both stage B tests unsatisfiable - every screw axis passed unchallenged and the centering rescue switched itself off on exactly the weak data it exists for. Where the fixed cut is the smaller of the two they are equal and this is inert: over the 37-crystal rotation battery every crystal reports the identical space group and identical merge statistics, so it is a no-op there and the low-ISa case it targets remains unmeasured. rugnux: --polarization reaches --mode azint, which parsed the flag and then dropped it; that mode also applies the same polarization default as every other mode. Acknowledge the ACTS/traccc project, whose sparse connected-component labelling both spot extractors take their algorithm from, with its citation and its license. The rc.161 change list is brought back to one line per entry, and the user-visible changes that were missing from it added. Co-Authored-By: Claude Opus 5 (1M context) --- acquisition_device/AcquisitionDevice.cpp | 7 +- broker/OpenAPIConvert.cpp | 11 +- broker/gen/model/Spot_finding_settings.cpp | 23 +- broker/gen/model/Spot_finding_settings.h | 6 +- broker/jfjoch_api.yaml | 5 +- broker/redoc-static.html | 5 +- common/DiffractionExperiment.cpp | 43 +- common/DiffractionExperiment.h | 5 + docs/ACKNOWLEDGEMENT.md | 9 + docs/CHANGELOG.md | 68 ++-- docs/RUGNUX.md | 2 +- docs/THIRD_PARTY_NOTICES.md | 10 +- .../python_client/docs/SpotFindingSettings.md | 2 +- frame_serialize/CBORStream2Deserializer.cpp | 10 +- frontend/src/client/types.gen.ts | 6 +- frontend/src/client/zod.gen.ts | 2 +- image_analysis/MXAnalysisWithoutFPGA.cpp | 8 +- image_analysis/MXAnalysisWithoutFPGA.h | 2 +- .../geom_refinement/AssignSpotsToRings.cpp | 28 +- .../geom_refinement/AssignSpotsToRings.h | 15 +- image_analysis/geom_refinement/Calibrants.cpp | 14 +- image_analysis/geom_refinement/Calibrants.h | 4 + .../ImagePreprocessorGPU.cu | 35 +- .../scale_merge/SearchSpaceGroup.cpp | 17 +- .../spot_finding/DetModuleSpotFinder_cpu.h | 3 +- .../spot_finding/SpotExtractorGPU.cu | 14 +- .../spot_finding/SpotFindingSettings.h | 7 +- licenses/COLLECT.sh | 2 +- licenses/traccc.txt | 373 ++++++++++++++++++ reader/HDF5ImageLocator.cpp | 25 +- reader/HDF5ImageLocator.h | 7 +- reader/HDF5ImageSource.cpp | 5 +- reader/HDF5ImageSource.h | 3 +- reader/JFJochHDF5Reader.cpp | 5 +- reader/JFJochHDF5Reader.h | 6 +- rugnux/Rugnux.cpp | 10 +- rugnux/rugnux_cli.cpp | 38 +- tests/CBORTest.cpp | 21 +- tests/CalibrationTest.cpp | 46 ++- tests/JFJochReaderTest.cpp | 118 ++++++ viewer/JFJochImageReadingWorker.cpp | 12 +- viewer/widgets/JFJochViewerSettingsDock.cpp | 6 +- 42 files changed, 880 insertions(+), 158 deletions(-) create mode 100644 licenses/traccc.txt diff --git a/acquisition_device/AcquisitionDevice.cpp b/acquisition_device/AcquisitionDevice.cpp index 00fecac8..a422fb58 100644 --- a/acquisition_device/AcquisitionDevice.cpp +++ b/acquisition_device/AcquisitionDevice.cpp @@ -320,9 +320,14 @@ void AcquisitionDevice::RunInternalGenerator(const DiffractionExperiment &experi void AcquisitionDevice::SetSpotFinderParameters(const SpotFindingSettings &settings) { SpotFinderParameters fpga_parameters{}; + // The FPGA compares d against these as xray_d_t = ap_ufixed<16,9> (hls_jfjoch.h), which tops out just + // below 512 A and WRAPS above it - so "no low-resolution limit" is that ceiling, not infinity. Sending + // a larger number would wrap to a small d and mask the whole image. The high end needs no such care: + // no pixel has d < 0. + constexpr float FPGA_MAX_D_A = 511.0f; fpga_parameters.snr_threshold = settings.signal_to_noise_threshold; fpga_parameters.count_threshold = settings.photon_count_threshold; - fpga_parameters.max_d = settings.low_resolution_limit; + fpga_parameters.max_d = settings.low_resolution_limit.value_or(FPGA_MAX_D_A); fpga_parameters.min_d = settings.high_resolution_limit.value_or(0.0f); fpga_parameters.min_pix_per_spot = settings.min_pix_per_spot.value_or(2); HW_SetSpotFinderParameters(fpga_parameters); diff --git a/broker/OpenAPIConvert.cpp b/broker/OpenAPIConvert.cpp index 50a2bf06..7fe98181 100644 --- a/broker/OpenAPIConvert.cpp +++ b/broker/OpenAPIConvert.cpp @@ -16,9 +16,13 @@ SpotFindingSettings Convert(const org::openapitools::server::model::Spot_finding ret.photon_count_threshold = input.getPhotonCountThreshold(); ret.min_pix_per_spot = input.getMinPixPerSpot(); ret.max_pix_per_spot = input.getMaxPixPerSpot(); - if (input.highResolutionLimitIsSet()) + // Both limits are optional and unset means "no limit at that end". A value of 0 has meant the same + // thing since rc.161 and clients still send it that way, so fold it into the unset case here - the + // analysis code then has exactly one spelling for "no limit" and no zero to special-case. + if (input.highResolutionLimitIsSet() && input.getHighResolutionLimit() > 0) ret.high_resolution_limit = input.getHighResolutionLimit(); - ret.low_resolution_limit = input.getLowResolutionLimit(); + if (input.lowResolutionLimitIsSet() && input.getLowResolutionLimit() > 0) + ret.low_resolution_limit = input.getLowResolutionLimit(); ret.enable = input.isEnable(); ret.indexing = input.isIndexing(); ret.quick_integration = input.isQuickIntegration(); @@ -41,7 +45,8 @@ org::openapitools::server::model::Spot_finding_settings Convert(const SpotFindin ret.setMaxPixPerSpot(input.max_pix_per_spot); if (input.high_resolution_limit.has_value()) ret.setHighResolutionLimit(input.high_resolution_limit.value()); - ret.setLowResolutionLimit(input.low_resolution_limit); + if (input.low_resolution_limit.has_value()) + ret.setLowResolutionLimit(input.low_resolution_limit.value()); ret.setEnable(input.enable); ret.setIndexing(input.indexing); ret.setHighResolutionLimitForSpotCountLowRes(input.cutoff_spot_count_low_res); diff --git a/broker/gen/model/Spot_finding_settings.cpp b/broker/gen/model/Spot_finding_settings.cpp index d85be081..e96079f1 100644 --- a/broker/gen/model/Spot_finding_settings.cpp +++ b/broker/gen/model/Spot_finding_settings.cpp @@ -30,6 +30,7 @@ Spot_finding_settings::Spot_finding_settings() m_High_resolution_limit = 0.0f; m_High_resolution_limitIsSet = false; m_Low_resolution_limit = 0.0f; + m_Low_resolution_limitIsSet = false; m_High_resolution_limit_for_spot_count_low_res = 0.0f; m_Quick_integration = false; m_Ice_ring_width_q_recipA = 0.03f; @@ -223,8 +224,8 @@ bool Spot_finding_settings::operator==(const Spot_finding_settings& rhs) const ((!highResolutionLimitIsSet() && !rhs.highResolutionLimitIsSet()) || (highResolutionLimitIsSet() && rhs.highResolutionLimitIsSet() && getHighResolutionLimit() == rhs.getHighResolutionLimit())) && - (getLowResolutionLimit() == rhs.getLowResolutionLimit()) - && + + ((!lowResolutionLimitIsSet() && !rhs.lowResolutionLimitIsSet()) || (lowResolutionLimitIsSet() && rhs.lowResolutionLimitIsSet() && getLowResolutionLimit() == rhs.getLowResolutionLimit())) && (getHighResolutionLimitForSpotCountLowRes() == rhs.getHighResolutionLimitForSpotCountLowRes()) && @@ -263,7 +264,8 @@ void to_json(nlohmann::json& j, const Spot_finding_settings& o) j["max_pix_per_spot"] = o.m_Max_pix_per_spot; if(o.highResolutionLimitIsSet()) j["high_resolution_limit"] = o.m_High_resolution_limit; - j["low_resolution_limit"] = o.m_Low_resolution_limit; + if(o.lowResolutionLimitIsSet()) + j["low_resolution_limit"] = o.m_Low_resolution_limit; j["high_resolution_limit_for_spot_count_low_res"] = o.m_High_resolution_limit_for_spot_count_low_res; j["quick_integration"] = o.m_Quick_integration; j["ice_ring_width_q_recipA"] = o.m_Ice_ring_width_q_recipA; @@ -289,7 +291,11 @@ void from_json(const nlohmann::json& j, Spot_finding_settings& o) j.at("high_resolution_limit").get_to(o.m_High_resolution_limit); o.m_High_resolution_limitIsSet = true; } - j.at("low_resolution_limit").get_to(o.m_Low_resolution_limit); + if(j.find("low_resolution_limit") != j.end()) + { + j.at("low_resolution_limit").get_to(o.m_Low_resolution_limit); + o.m_Low_resolution_limitIsSet = true; + } j.at("high_resolution_limit_for_spot_count_low_res").get_to(o.m_High_resolution_limit_for_spot_count_low_res); j.at("quick_integration").get_to(o.m_Quick_integration); j.at("ice_ring_width_q_recipA").get_to(o.m_Ice_ring_width_q_recipA); @@ -383,6 +389,15 @@ float Spot_finding_settings::getLowResolutionLimit() const void Spot_finding_settings::setLowResolutionLimit(float const value) { m_Low_resolution_limit = value; + m_Low_resolution_limitIsSet = true; +} +bool Spot_finding_settings::lowResolutionLimitIsSet() const +{ + return m_Low_resolution_limitIsSet; +} +void Spot_finding_settings::unsetLow_resolution_limit() +{ + m_Low_resolution_limitIsSet = false; } float Spot_finding_settings::getHighResolutionLimitForSpotCountLowRes() const { diff --git a/broker/gen/model/Spot_finding_settings.h b/broker/gen/model/Spot_finding_settings.h index 63838b1b..873d2d09 100644 --- a/broker/gen/model/Spot_finding_settings.h +++ b/broker/gen/model/Spot_finding_settings.h @@ -95,10 +95,12 @@ public: bool highResolutionLimitIsSet() const; void unsetHigh_resolution_limit(); /// - /// Low resolution limit for spot finding [Angstrom] + /// Low resolution limit for spot finding [Angstrom]. Optional: if omitted, spot finding is not clipped at the low-resolution end. A value of 0 is accepted and means the same thing. /// float getLowResolutionLimit() const; void setLowResolutionLimit(float const value); + bool lowResolutionLimitIsSet() const; + void unsetLow_resolution_limit(); /// /// High resolution threshold to consider spot \"low resolution\" [Angstrom] /// @@ -154,7 +156,7 @@ protected: float m_High_resolution_limit; bool m_High_resolution_limitIsSet; float m_Low_resolution_limit; - + bool m_Low_resolution_limitIsSet; float m_High_resolution_limit_for_spot_count_low_res; bool m_Quick_integration; diff --git a/broker/jfjoch_api.yaml b/broker/jfjoch_api.yaml index eb75e3ea..236eee22 100644 --- a/broker/jfjoch_api.yaml +++ b/broker/jfjoch_api.yaml @@ -1038,7 +1038,6 @@ components: - photon_count_threshold - max_pix_per_spot - min_pix_per_spot - - low_resolution_limit - quick_integration - high_resolution_limit_for_spot_count_low_res - ice_ring_width_q_recipA @@ -1079,7 +1078,9 @@ components: low_resolution_limit: type: number format: float - description: Low resolution limit for spot finding [Angstrom] + description: | + Low resolution limit for spot finding [Angstrom]. Optional: if omitted, spot finding is not + clipped at the low-resolution end. A value of 0 is accepted and means the same thing. high_resolution_limit_for_spot_count_low_res: type: number format: float diff --git a/broker/redoc-static.html b/broker/redoc-static.html index 7d084587..f40ccb83 100644 --- a/broker/redoc-static.html +++ b/broker/redoc-static.html @@ -710,7 +710,8 @@ Even if disabled spot finding information will still be send and written, though indexing
required
boolean
Default: true

Enable indexing. This is temporary setting, i.e. can be changed anytime during data collection.

signal_to_noise_threshold
required
number <float> >= 0
photon_count_threshold
required
integer <int64> >= 0
min_pix_per_spot
required
integer <int64> >= 1
max_pix_per_spot
required
integer <int64> >= 1
high_resolution_limit
number <float>

High resolution limit for spot finding [Angstrom]. Optional: if omitted, spot finding extends as far as the detector reaches, i.e. the detection is not clipped in resolution.

-
low_resolution_limit
required
number <float>

Low resolution limit for spot finding [Angstrom]

+
low_resolution_limit
number <float>

Low resolution limit for spot finding [Angstrom]. Optional: if omitted, spot finding is not +clipped at the low-resolution end. A value of 0 is accepted and means the same thing.

high_resolution_limit_for_spot_count_low_res
required
number <float> [ 2 .. 8 ]

High resolution threshold to consider spot "low resolution" [Angstrom]

quick_integration
required
boolean
Default: false

Quick integration of Bragg spots in diffraction images. If enabled it will likely reduce performance of Jungfraujoch for datasets with a very high indexing rate. @@ -976,7 +977,7 @@ then image might be replaced in the buffer between calling /images and /image.cb