From a97d763098984f251c24da50fd8a68999e2354ba Mon Sep 17 00:00:00 2001 From: jungfrau Date: Tue, 18 Aug 2026 17:48:19 -0400 Subject: [PATCH] Do not assume a DECTRIS detector's bit depths before asking it DetectorSetup gave a DECTRIS detector bit_depth_image = 16 and bit_depth_readout = 16 the moment it was constructed. Both mean "the detector told us", and at construction nothing has asked it: the SIMPLON client learns them at configure time, and the lite receiver reads them off the start message. Until then they were an assumption wearing the clothes of a measurement. The assumption was load-bearing in the wrong direction. GetByteDepthImage() gives the detector's image depth absolute priority - correctly, since the DECTRIS path forwards images verbatim and the depth has to be the one the pixels actually have - so a value that was always set meant everything below it was unreachable on DECTRIS: * image_format_settings.bit_depth_image, which the API documents as "bit depth of resulting image ... if not provided value is adjusted automatically", was silently discarded. It works on JUNGFRAU, where the detector's value is not set, and did nothing at all on an EIGER. No error, no warning. * so was the promotion to four bytes that summation needs. Both are now unset for DECTRIS. JUNGFRAU keeps its readout depth of 16, because there that is a property of the hardware rather than something configured. Where nothing supplies a depth the answer does not change: GetByteDepthImage() falls through to the same two bytes it produced before, and the fallback no longer throws merely because the readout depth is not known yet - GetBitDepthReadoutIfKnown() reports it as unknown instead of inventing one. The start message carries it as an optional already, and the receiver already guards on that, so "not known" travels end to end rather than being papered over. GetBitDepthReadout() still throws for the callers that genuinely require a value; all of them are on the FPGA path, where it is always set. Also fixes the jfjoch_test build, which the previous commit broke: SpotExtractorGPU's Extract() takes a PixelView now, and the parity test still passed a raw pointer. That should have been caught before it was pushed. Tests: DetectorSetup (with a new case pinning the contract this restores), plus DiffractionExperiment, CBOR, writer, HDF5, preprocessing, azimuthal integration and the GPU spot-finding / integration / decoder suites. rugnux unchanged end to end. Co-Authored-By: Claude Opus 5 --- common/DetectorSetup.cpp | 14 ++++++++++-- common/DiffractionExperiment.cpp | 19 ++++++++++------ common/DiffractionExperiment.h | 3 +++ docs/CHANGELOG.md | 1 + tests/DetectorSetupTest.cpp | 33 +++++++++++++++++++++++++++- tests/SpotExtractorGPUParityTest.cpp | 2 +- 6 files changed, 61 insertions(+), 11 deletions(-) diff --git a/common/DetectorSetup.cpp b/common/DetectorSetup.cpp index f6b2b3cc..73429dd8 100644 --- a/common/DetectorSetup.cpp +++ b/common/DetectorSetup.cpp @@ -74,8 +74,18 @@ DetectorSetup::DetectorSetup(std::shared_ptr in_geometry, break; case DetectorType::DECTRIS: high_voltage = 0; - bit_depth_readout = 16; - bit_depth_image = 16; + // Neither depth is set here. Both mean "the detector told us", and until something has + // asked it - the SIMPLON client at configure time, or the start message off the stream - + // nobody has. JUNGFRAU above keeps its 16, because there a 16-bit readout is a property + // of the hardware rather than something that gets configured. + // + // bit_depth_image in particular is deliberately left unset. It means "the detector told us what depth + // its images are", and until something has asked the detector - or read it off the + // stream - nobody has. Defaulting it to 16 made it look answered, and since + // GetByteDepthImage() gives this value absolute priority, that silently shadowed both + // the image-format setting the API offers and the promotion to 32 bits that summation + // needs. Unset, the fallback still lands on 16 bits for a DECTRIS detector, via the + // readout depth just above. read_out_time = std::chrono::microseconds(0); if (!det_modules_hostname.empty() && ( det_modules_hostname.size() != 1)) throw JFJochException(JFJochExceptionCategory::InputParameterInvalid, diff --git a/common/DiffractionExperiment.cpp b/common/DiffractionExperiment.cpp index 1015bd0b..0b424a4e 100644 --- a/common/DiffractionExperiment.cpp +++ b/common/DiffractionExperiment.cpp @@ -392,9 +392,12 @@ int64_t DiffractionExperiment::GetByteDepthImage() const { auto bit_depth_image = image_format_settings.GetBitDepthImage(); if (!bit_depth_image.has_value()) { - if (GetBitDepthReadout() == 32) + // An unknown readout depth is not an error here - it just leaves the two-byte default below, + // which is what a DECTRIS detector reads out until it says otherwise. + const auto readout = GetBitDepthReadoutIfKnown(); + if (readout == 32) return 4; - if (GetBitDepthReadout() == 8) + if (readout == 8) return 1; return (GetSummation() > 2) ? 4 : 2; } @@ -669,7 +672,7 @@ void DiffractionExperiment::FillMessage(StartMessage &message) const { message.sensor_material = detector.GetSensorMaterial(); message.sensor_thickness = detector.GetSensorThickness_um() * 1e-6f; message.bit_depth_image = GetByteDepthImage() * 8; - message.bit_depth_readout = GetBitDepthReadout(); + message.bit_depth_readout = GetBitDepthReadoutIfKnown(); message.indexing_algorithm = GetIndexingAlgorithm(); message.images_per_trigger = dataset.GetImageNumPerTrigger(); @@ -1216,13 +1219,15 @@ int64_t DiffractionExperiment::GetImageFillValue() const { } } -int64_t DiffractionExperiment::GetBitDepthReadout() const { +std::optional DiffractionExperiment::GetBitDepthReadoutIfKnown() const { if (GetDetectorType() == DetectorType::EIGER) return GetEigerBitDepth(); + return detector.GetBitDepthReadout(); +} - auto det_value = detector.GetBitDepthReadout(); - if (det_value) - return det_value.value(); +int64_t DiffractionExperiment::GetBitDepthReadout() const { + if (auto value = GetBitDepthReadoutIfKnown()) + return value.value(); throw JFJochException(JFJochExceptionCategory::InputParameterInvalid, "Bit depth readout not configured"); } diff --git a/common/DiffractionExperiment.h b/common/DiffractionExperiment.h index a1f3dafd..d7abd51b 100644 --- a/common/DiffractionExperiment.h +++ b/common/DiffractionExperiment.h @@ -203,6 +203,9 @@ public: DetectorMode GetDetectorMode() const; int64_t GetBitDepthReadout() const; // 12 bit is OK :) + // The same, for the callers that have to cope with not knowing it yet: a DECTRIS detector only + // learns its readout depth when something asks the detector or reads the start message. + std::optional GetBitDepthReadoutIfKnown() const; int64_t GetSaturationLimit() const; int64_t GetOverflow() const; diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 7d83164f..81fc3068 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -1,6 +1,7 @@ # Changelog ## 1.0.0 ### Unreleased +* jfjoch_broker: a DECTRIS detector no longer claims a 16-bit image and readout depth before anything has asked the detector. Both are now unset until the SIMPLON configuration or the incoming stream supplies them. **This makes `image_format_settings.bit_depth_image` work on DECTRIS detectors** - it was documented as controlling the output depth, but the detector's assumed default took priority and it was silently discarded. Where nothing supplies a depth the result is unchanged (16-bit). * **rugnux: 16-bit images keep their width all the way through the GPU pipeline** instead of being widened to 32-bit as soon as they are decoded. Every per-pixel pass over a frame - the ring statistics, the strong-pixel search, spot extraction, azimuthal and ROI integration, Bragg integration - then moves half as many bytes. This is the mode a fast acquisition reads out in, and the gain grows with the detector: on the 2.5M-pixel dataset in the test set the image loop drops 2%, and it is worth proportionally more on a large detector, where per-pixel work is most of the loop. Merged results are unchanged. * **rugnux: the first pass of the rotation two-pass no longer merges, reports or writes anything.** It exists to measure the detector geometry, the goniometer rotation scale, the mosaicity and the space group, and the second pass makes the merged result again at the refined geometry - so the first pass now stops once it has those. **The `_01_*` files are no longer written.** A battery of 24 rotation crystals drops from 6m52s to 6m17s and no merged result changes. * rugnux: every run ends with a `Time by phase` table - wall time and mean cores busy for each phase of the whole run, both passes together - so a slow dataset can be diagnosed from its own log without a profiler. diff --git a/tests/DetectorSetupTest.cpp b/tests/DetectorSetupTest.cpp index 5e81b8c5..606d0bed 100644 --- a/tests/DetectorSetupTest.cpp +++ b/tests/DetectorSetupTest.cpp @@ -4,6 +4,7 @@ #include #include "../common/DetectorSetup.h" #include "../common/NetworkAddressConvert.h" +#include "../common/DiffractionExperiment.h" TEST_CASE("DetectorSetup_MismatchInSize") { REQUIRE_THROWS( @@ -194,4 +195,34 @@ TEST_CASE("DetectorSetup_MaxFrameTime") { DetectorSetup s2(DetectorGeometryModular(8), DetectorType::EIGER); REQUIRE(s2.GetMinFrameTime() == MIN_FRAME_TIME_EIGER); -} \ No newline at end of file +} +// A DECTRIS detector starts with no image depth of its own, so the depth comes from what the run is +// actually configured to do. It used to default to 16, and because GetByteDepthImage() gives the +// detector's value absolute priority that silently shadowed everything below it - the image-format +// setting the API offers among them. +TEST_CASE("DetectorSetup_ImageDepthDefaultsToUnset") { + auto setup = DetDECTRIS(123, 123, "zzz", "a"); + REQUIRE(!setup.GetBitDepthImage()); + REQUIRE(!setup.GetBitDepthReadout()); + + // Nothing set anywhere: the fallback still lands on the readout depth, as before. + DiffractionExperiment x(setup); + REQUIRE(x.GetByteDepthImage() == 2); + + // The API's image-format setting is now reached instead of being discarded. + x.ImportImageFormatSettings(ImageFormatSettings().BitDepthImage(32)); + REQUIRE(x.GetByteDepthImage() == 4); + + // An unknown readout depth is not an error while the image depth is being decided, and it is + // reported as "not known" rather than invented. + REQUIRE_THROWS(x.GetBitDepthReadout()); + REQUIRE(!x.GetBitDepthReadoutIfKnown()); + setup.BitDepthReadout(32); + REQUIRE(DiffractionExperiment(setup).GetByteDepthImage() == 4); + + // And a depth the detector itself reports still wins over both. + setup.BitDepthImage(8); + DiffractionExperiment y(setup); + y.ImportImageFormatSettings(ImageFormatSettings().BitDepthImage(32)); + REQUIRE(y.GetByteDepthImage() == 1); +} diff --git a/tests/SpotExtractorGPUParityTest.cpp b/tests/SpotExtractorGPUParityTest.cpp index b5a86d9b..bdefce78 100644 --- a/tests/SpotExtractorGPUParityTest.cpp +++ b/tests/SpotExtractorGPUParityTest.cpp @@ -190,7 +190,7 @@ public: } std::vector Gpu(const SpotFindingSettings &settings) { std::vector spots; - extractor.Extract(gpu_strong.get(), image.getGPUBuffer(), settings, spots); + extractor.Extract(gpu_strong.get(), ViewOf(image), settings, spots); return spots; } };