From c6f6396e462006185edd1e6ad30c072449052a37 Mon Sep 17 00:00:00 2001 From: Filip Leonarski Date: Sat, 10 Oct 2026 18:56:00 +0200 Subject: [PATCH 1/4] Indexer failures are thrown, not read as "no lattice"; GPU memory errors say what to do A CUDA out-of-memory in an indexer worker was turned into an IndexerResult carrying only the message string. The rotation indexer then returned no lattice, the first pass treated that as a genuine negative and the rescues acted on it: in a stress run (two rugnux on one card) a run had both first-pass schemes fail with out-of-memory, the axis-sign rescue compared 0 frames at the flipped sign with the -1 "no lattice" sentinel, adopted the flipped sign, and the beam-centre rescue then moved the centre - a failure of the machine used as evidence about the crystal. The CUDA category was lost on the way, so the fail-fast check (IsFatalResourceError) never saw it. - IndexerResult::error is now the std::exception_ptr the worker caught. RotationIndexer::RunIndexing rethrows it before touching any state, so nothing is memoised for a failed run and the exception keeps its category: rugnux stops with the CUDA message at once (verified with a forced indexer failure: exit 1 after 1.9 s, "CUDA (GPU) error (...)"). FirstPass::indexer_error, RotationIndexer::GetIndexerError and RequireLattice's machine-failure branch are gone. - The beam-centre arm in RunAllPasses rethrows fatal resource errors like its sibling catches; it had caught the out-of-memory and carried on at the file's centre. - IndexAndRefine keeps per-frame indexer failures apart from frames that did not index (count + first exception). The frame stays "not attempted" as before; the receiver logs the count and the first error at the end of a collection (no cancel), rugnux's image pass and stills geometry refinement rethrow it and stop. A rotation-indexer failure inside IndexAndRefine (online rotation indexing, FinalizeRotationIndexing) is recorded the same way, so a live collection is not cancelled by it. - A failed device allocation or cuFFT plan now says how much was asked, how much of the card is free, and - when the card is merely full - that another program is using the GPU or the data set needs more than the card has, with the CPU remedy; previously "Failed to allocate device memory (out of memory)" / "Failed to create CUFFT plan with cufftPlanMany". Co-Authored-By: Claude Opus 5.5 (1M context) --- image_analysis/IndexAndRefine.cpp | 37 +++++++++++++++-- image_analysis/IndexAndRefine.h | 13 ++++++ image_analysis/indexing/CUDAMemHelpers.h | 40 ++++++++++++++++--- image_analysis/indexing/Indexer.h | 12 +++--- image_analysis/indexing/IndexerThreadPool.cpp | 6 ++- .../rotation_indexer/RotationIndexer.cpp | 16 +++----- .../rotation_indexer/RotationIndexer.h | 8 +--- receiver/JFJochReceiver.cpp | 11 +++++ rugnux/RugnuxFirstPass.cpp | 2 - rugnux/RugnuxFirstPass.h | 7 ++-- rugnux/RugnuxFirstPassAccept.cpp | 14 +------ rugnux/RugnuxImagePass.cpp | 7 +++- rugnux/RugnuxPasses.cpp | 4 +- rugnux/RugnuxScaleMerge.cpp | 4 +- rugnux/RugnuxStills.cpp | 1 + tests/CudaHandledErrorTest.cpp | 4 +- tests/RotationIndexerTest.cpp | 3 -- 17 files changed, 131 insertions(+), 58 deletions(-) diff --git a/image_analysis/IndexAndRefine.cpp b/image_analysis/IndexAndRefine.cpp index 145dad247..ef313f71f 100644 --- a/image_analysis/IndexAndRefine.cpp +++ b/image_analysis/IndexAndRefine.cpp @@ -88,6 +88,24 @@ std::optional IndexAndRefine::RotationAngle(int64_t image) const { return std::nullopt; } +void IndexAndRefine::RecordIndexerError(std::exception_ptr e) { + std::lock_guard lock(indexer_error_mutex_); + if (!first_indexer_error_) + first_indexer_error_ = e; + ++indexer_errors_; +} + +int64_t IndexAndRefine::GetIndexerErrorCount() const { + std::lock_guard lock(indexer_error_mutex_); + return indexer_errors_; +} + +void IndexAndRefine::RethrowIndexerError() const { + std::lock_guard lock(indexer_error_mutex_); + if (first_indexer_error_) + std::rethrow_exception(first_indexer_error_); +} + void IndexAndRefine::AddImageToRotationIndexer(DataMessage &msg) { if (rotation_indexer) rotation_indexer->ProcessImage(msg.number, msg.spots, RotationAngle(msg.number)); @@ -105,8 +123,13 @@ IndexAndRefine::IndexingOutcome IndexAndRefine::DetermineLatticeAndSymmetryRotat auto rot_cnt = rotation_indexer_counter.Process(msg.number); if (rot_cnt.first) rotation_indexer->ProcessImage(msg.number, msg.spots, RotationAngle(msg.number)); - if (rot_cnt.second) - rotation_indexer->RunIndexing(); + if (rot_cnt.second) { + try { + rotation_indexer->RunIndexing(); + } catch (...) { + RecordIndexerError(std::current_exception()); + } + } result = rotation_indexer->GetLattice(); } @@ -187,6 +210,8 @@ IndexAndRefine::IndexingOutcome IndexAndRefine::DetermineLatticeAndSymmetry(Data } } auto res = indexer_->Run(experiment, recip); + if (res.error) + RecordIndexerError(res.error); any_executed |= res.executed; // Kept from whichever run could measure it - the largest seed that answered - rather than from // the run whose lattice won: the severity describes the frame's lattice rows, not the cell that @@ -247,6 +272,8 @@ IndexAndRefine::IndexingOutcome IndexAndRefine::DetermineLatticeAndSymmetry(Data const auto algorithm = experiment.GetIndexingAlgorithm(); if (algorithm == IndexingAlgorithmEnum::FFT || algorithm == IndexingAlgorithmEnum::FFTW) { const auto res = indexer_->Run(experiment, recip, /*severity_only=*/true); + if (res.error) + RecordIndexerError(res.error); spindle_blind_fraction = res.spindle_blind_fraction; } if (!spindle_blind_fraction && !indexer_result.lattice.empty()) { @@ -847,7 +874,11 @@ std::optional IndexAndRefine::FinalizeRotationIndexing() if (const auto latt = rotation_indexer->GetLattice()) return latt; - rotation_indexer->RunIndexing(); + try { + rotation_indexer->RunIndexing(); + } catch (...) { + RecordIndexerError(std::current_exception()); + } return rotation_indexer->GetLattice(); } return {}; diff --git a/image_analysis/IndexAndRefine.h b/image_analysis/IndexAndRefine.h index 541603d40..e6bee48e5 100644 --- a/image_analysis/IndexAndRefine.h +++ b/image_analysis/IndexAndRefine.h @@ -5,6 +5,7 @@ #include #include +#include #include #include #include @@ -173,6 +174,13 @@ class IndexAndRefine { const IndexingOutcome &outcome, const BraggIntegrateFn &probe_integrate); + // Where the indexer failed rather than found nothing: how often, and the first failure as thrown. + // The frame is left unindexed, as one the indexer never got to; the caller decides the rest. + mutable std::mutex indexer_error_mutex_; + int64_t indexer_errors_ = 0; + std::exception_ptr first_indexer_error_; + void RecordIndexerError(std::exception_ptr e); + std::unique_ptr reindex_resolver; void ScaleImage(DataMessage &msg, IntegrationOutcome& outcome); @@ -186,6 +194,11 @@ public: bool real_time = false); void AddImageToRotationIndexer(DataMessage &msg); + // An indexer that fails says nothing about the frame and everything about the machine, so it is + // not a frame that did not index. Online, a collection goes on and reports how often it happened + // (GetIndexerErrorCount); offline, the run stops on it (RethrowIndexerError). + int64_t GetIndexerErrorCount() const; + void RethrowIndexerError() const; // the first failure, if there was one void ForceRotationIndexerLattice(const CrystalLattice& lattice); void ForceRotationIndexerResult(const RotationIndexerResult& result); // Supply a per-frame (by image number) mosaicity for prediction, overriding the per-image estimate. diff --git a/image_analysis/indexing/CUDAMemHelpers.h b/image_analysis/indexing/CUDAMemHelpers.h index 27e167491..8945c0fc1 100644 --- a/image_analysis/indexing/CUDAMemHelpers.h +++ b/image_analysis/indexing/CUDAMemHelpers.h @@ -5,13 +5,37 @@ #include #include +#include #include #include #include +#include #include #include "../common/JFJochException.h" #include "../common/CUDAWrapper.h" +// The message for GPU memory that could not be had: what was asked (bytes, 0 where unknown), what the +// card has free and, where it is merely full, the usual causes and the remedy - the same advice the +// scaling's own out-of-memory gives. A context lost to another fault fails here too and keeps its reason. +inline std::string GPUMemoryFailure(const char *what, size_t bytes, bool out_of_memory, const char *reason) { + char buf[512]; + if (bytes > 0) + std::snprintf(buf, sizeof buf, "%s (%.1f MB) failed (%s)", what, bytes / 1e6, reason); + else + std::snprintf(buf, sizeof buf, "%s failed (%s)", what, reason); + std::string msg(buf); + if (!out_of_memory) + return msg; + size_t free_b = 0, total_b = 0; + if (cudaMemGetInfo(&free_b, &total_b) == cudaSuccess) { + std::snprintf(buf, sizeof buf, ", with %.1f of the %.1f GB card free", free_b / 1e9, total_b / 1e9); + msg += buf; + } + return msg + ". Either another program is using this GPU, or this data set needs more GPU memory than " + "the card has - free the GPU and run again, or run on the CPU (CUDA_VISIBLE_DEVICES= rugnux " + "..., or a CPU-only build)"; +} + class CudaStream { cudaStream_t stream_ = nullptr; public: @@ -94,12 +118,15 @@ public: const int* onembed, int ostride, int odist, cufftType type, int batch) { - if (cufftPlanMany(&plan_, rank, const_cast(n), - const_cast(inembed), istride, idist, - const_cast(onembed), ostride, odist, - type, batch) != CUFFT_SUCCESS) + const cufftResult r = cufftPlanMany(&plan_, rank, const_cast(n), + const_cast(inembed), istride, idist, + const_cast(onembed), ostride, odist, + type, batch); + if (r != CUFFT_SUCCESS) throw JFJochException(JFJochExceptionCategory::GPUCUDAError, - "Failed to create CUFFT plan with cufftPlanMany"); + GPUMemoryFailure("Creating the CUFFT plan (cufftPlanMany)", 0, + r == CUFFT_ALLOC_FAILED, + ("cuFFT error " + std::to_string(r)).c_str())); } // Convenience overload for vector input @@ -314,7 +341,8 @@ public: // as a card that is merely full. if (const cudaError_t err = cudaMalloc(&ptr_, count * sizeof(T)); err != cudaSuccess) throw JFJochException(JFJochExceptionCategory::GPUCUDAError, - std::string("Failed to allocate device memory (") + cudaGetErrorString(err) + ")"); + GPUMemoryFailure("Allocating GPU memory", count * sizeof(T), + err == cudaErrorMemoryAllocation, cudaGetErrorString(err))); } ~CudaDevicePtr() { release(); diff --git a/image_analysis/indexing/Indexer.h b/image_analysis/indexing/Indexer.h index cd2190145..8ed46ee6f 100644 --- a/image_analysis/indexing/Indexer.h +++ b/image_analysis/indexing/Indexer.h @@ -3,6 +3,7 @@ #pragma once +#include #include #include "../../common/CrystalLattice.h" @@ -16,11 +17,12 @@ struct IndexerResult { std::vector lattice; float indexing_time_s; bool executed = false; // If indexing was not performed (due to indexer being not-available) mark it, so indexing result is marked accordingly - // What the indexer threw, when it threw. A THIRD outcome, not a flavour of the other two: an - // empty lattice list with no error is a real negative result about the frame - it was examined - // and no lattice fits it - whereas an error says nothing about the frame at all, the indexer - // never having got to look, and will recur on the next one. - std::optional error; + // What the indexer threw, when it threw - the exception itself, so its category (a CUDA + // out-of-memory, say) reaches whoever decides what to do about it. A THIRD outcome, not a flavour + // of the other two: an empty lattice list with no error is a real negative result about the + // frame - it was examined and no lattice fits it - whereas an error says nothing about the frame + // at all, the indexer never having got to look, and will recur on the next one. + std::exception_ptr error; // How much of a single sweep's blind cone this orientation makes unrecoverable, in [0,1]. // Absent when the frame gives nothing to decide on (no spindle defined, no lattice rows). std::optional spindle_blind_fraction; diff --git a/image_analysis/indexing/IndexerThreadPool.cpp b/image_analysis/indexing/IndexerThreadPool.cpp index ea1acdc62..4afe38a97 100644 --- a/image_analysis/indexing/IndexerThreadPool.cpp +++ b/image_analysis/indexing/IndexerThreadPool.cpp @@ -155,7 +155,8 @@ void IndexerThread::Worker(int threadid) { // downstream as "this frame did not index". spdlog::error("Indexer thread {} failed: {}", threadid, e.what()); tmp_result = std::make_unique(IndexerResult{ - .lattice = {}, .indexing_time_s = 0, .executed = false, .error = e.what()}); + .lattice = {}, .indexing_time_s = 0, .executed = false, + .error = std::current_exception()}); // This thread goes on to the next frame, so a CUDA error left behind here would be // reported by the first cudaGetLastError() of every frame that follows - one failed // attempt would read as an indexer that never works again. @@ -295,7 +296,8 @@ IndexerResult IndexerThreadPool::Run(const DiffractionExperiment &experiment, co } catch (const std::exception &e) { spdlog::error("Indexer thread failed: {}", e.what()); result = std::make_unique(IndexerResult{ - .lattice = {}, .indexing_time_s = 0, .executed = false, .error = e.what()}); + .lattice = {}, .indexing_time_s = 0, .executed = false, + .error = std::current_exception()}); } { std::unique_lock lock(m); diff --git a/image_analysis/rotation_indexer/RotationIndexer.cpp b/image_analysis/rotation_indexer/RotationIndexer.cpp index e077d828b..dbc1e168a 100644 --- a/image_analysis/rotation_indexer/RotationIndexer.cpp +++ b/image_analysis/rotation_indexer/RotationIndexer.cpp @@ -203,7 +203,7 @@ namespace { lattice(kb, b.indexed_lattice); for (const auto &l : a.extra_lattices) lattice(ka, l); for (const auto &l : b.extra_lattices) lattice(kb, l); - return ka == kb && a.indexer_error == b.indexer_error && a.tilt_walk.has_value() == b.tilt_walk.has_value() + return ka == kb && a.tilt_walk.has_value() == b.tilt_walk.has_value() && (a.unconstrained != nullptr) == (b.unconstrained != nullptr); } } @@ -236,8 +236,7 @@ std::vector RotationIndexer::InputKey() const { } RotationIndexer::IndexingOutcome RotationIndexer::Outcome() const { - return {axis_, updated_geom_, search_result_, extra_lattices_, unconstrained_, tilt_walk_, indexed_lattice, - indexer_error_}; + return {axis_, updated_geom_, search_result_, extra_lattices_, unconstrained_, tilt_walk_, indexed_lattice}; } void RotationIndexer::RunIndexing() { @@ -263,7 +262,6 @@ void RotationIndexer::RunIndexing() { unconstrained_ = kept->unconstrained; tilt_walk_ = kept->tilt_walk; indexed_lattice = kept->indexed_lattice; - indexer_error_ = kept->indexer_error; return; } @@ -295,7 +293,10 @@ void RotationIndexer::RunIndexingLocked() { coords.emplace_back(rot * s.ReciprocalCoord(geom_)); } const auto indexer_result = indexer_.Run(experiment, coords); - indexer_error_ = indexer_result.error; + // An indexer that failed never looked at the cloud, so there is no outcome to keep, and none is + // memoised (RunIndexing): the failure goes to the caller as the indexer threw it. + if (indexer_result.error) + std::rethrow_exception(indexer_result.error); tilt_walk_.reset(); if (!indexer_result.lattice.empty() && indexer_result.lattice[0].CalcVolume() > 1.0) { @@ -891,11 +892,6 @@ std::optional RotationIndexer::GetLattice() const { }; } -std::optional RotationIndexer::GetIndexerError() const { - std::unique_lock ul(m); - return indexer_error_; -} - void RotationIndexer::ForceResult(const RotationIndexerResult &result) { std::unique_lock ul(m); indexed_lattice = result.lattice; diff --git a/image_analysis/rotation_indexer/RotationIndexer.h b/image_analysis/rotation_indexer/RotationIndexer.h index fe83d261c..e17e22315 100644 --- a/image_analysis/rotation_indexer/RotationIndexer.h +++ b/image_analysis/rotation_indexer/RotationIndexer.h @@ -71,7 +71,6 @@ public: std::shared_ptr unconstrained; std::optional tilt_walk; std::optional indexed_lattice; - std::optional indexer_error; }; private: mutable std::mutex m; @@ -108,7 +107,6 @@ private: size_t accumulated_spots = 0; std::optional indexed_lattice; - std::optional indexer_error_; // The accumulated per-image spots as one reciprocal-space cloud, back-rotated to phi = 0 under the // given geometry and axis. The caller holds the lock. @@ -132,12 +130,10 @@ public: // `image` is used (only valid when `image` is the goniometer's own image index). void ProcessImage(int64_t image, const std::vector& spots, std::optional angle_deg = std::nullopt); + // Throws what the indexer threw, where it failed rather than found no lattice: the two are one + // about the machine and one about the crystal, and only the second is an answer. void RunIndexing(); std::optional GetLattice() const; - // What the indexer threw on the last RunIndexing(), if it threw. Empty when it simply found no - // lattice: a caller that reports the failure to the user needs the two apart, one being about - // the crystal and the other about the machine. - [[nodiscard]] std::optional GetIndexerError() const; void ForceLattice(const CrystalLattice& lattice); // Adopt a complete result (lattice + refined geometry/axis/symmetry) found by another indexer - // used when several first-pass sampling schemes are run and the best is selected. diff --git a/receiver/JFJochReceiver.cpp b/receiver/JFJochReceiver.cpp index 8c4aca8b3..abb36ea42 100644 --- a/receiver/JFJochReceiver.cpp +++ b/receiver/JFJochReceiver.cpp @@ -181,6 +181,17 @@ void JFJochReceiver::SendEndMessage() { rotation_indexing_lattice_type = message.rotation_lattice_type; } message.unit_cell = indexer.GetConsensusUnitCell(); + // The collection does not stop for an indexer that failed - the images are written regardless - but + // its indexing rate is then not a statement about the crystal, and the log has to say so. + if (const int64_t n = indexer.GetIndexerErrorCount(); n > 0) { + try { + indexer.RethrowIndexerError(); + } catch (const std::exception &e) { + logger.Error("Indexing failed {} times in this collection, first with: {}. Those attempts are " + "left out of the indexing rate, and a rotation lattice may be missing for the same " + "reason", n, e.what()); + } + } for (int i = 0; i < adu_histogram_module.size(); i++) message.adu_histogram["module" + std::to_string(i)] = adu_histogram_module[i]->GetHistogram(); diff --git a/rugnux/RugnuxFirstPass.cpp b/rugnux/RugnuxFirstPass.cpp index d2f814092..8aa476a60 100644 --- a/rugnux/RugnuxFirstPass.cpp +++ b/rugnux/RugnuxFirstPass.cpp @@ -817,8 +817,6 @@ auto Rugnux::FirstPassRun::ScoreSchemes(const SchemeIndexers &ris, IndexAndRefin for (size_t i = 0; i < ris.size(); i++) { if (rx.cancelled_) break; - if (!bp.indexer_error) - bp.indexer_error = ris[i]->GetIndexerError(); auto found = ris[i]->GetLattice(); if (!found.has_value()) continue; diff --git a/rugnux/RugnuxFirstPass.h b/rugnux/RugnuxFirstPass.h index 7ca66ce90..d9f6cc7c0 100644 --- a/rugnux/RugnuxFirstPass.h +++ b/rugnux/RugnuxFirstPass.h @@ -28,13 +28,12 @@ #include "../image_analysis/MXAnalysisWithoutFPGA.h" namespace rugnux_internal { - // A candidate lattice of the first pass and what it scored on the validation frames. - // indexer_error: what the indexer threw, if it threw. An empty result with no error is a - // verdict on the crystal; an empty result with one is a verdict on the machine. + // A candidate lattice of the first pass and what it scored on the validation frames; no result + // (score -1) where nothing indexed. An indexer that failed is not that: it throws (RunIndexing). // larger_settled: the result is the larger cell of an axis-harmonic pair, and the arbiter in // the scheme comparison established it on the spots. struct FirstPass { std::optional result; int score = -1; std::string name; double vol = 0.0; - std::optional indexer_error; bool larger_settled = false; }; + bool larger_settled = false; }; // Two results are one lattice when their Niggli-reduced primitive edges agree within 2 % (the // battery's lattice-identity test), whatever setting or class each is held in. diff --git a/rugnux/RugnuxFirstPassAccept.cpp b/rugnux/RugnuxFirstPassAccept.cpp index 55fd883f5..1df384d0b 100644 --- a/rugnux/RugnuxFirstPassAccept.cpp +++ b/rugnux/RugnuxFirstPassAccept.cpp @@ -103,20 +103,8 @@ namespace { } void Rugnux::FirstPassRun::RequireLattice(const FirstPass &best) const { - if (!best.result.has_value()) { - // An indexer that FAILED is a third outcome, and none of those remedies fit it: it - // never examined the frames, so nothing about the data or the processing options is - // implicated and changing them cannot help. Name the failure instead. - if (best.indexer_error) - throw JFJochException(JFJochExceptionCategory::IndexerError, - fmt::format("Rotation indexing could not run: {}. Nothing was " - "integrated. The indexer never examined the frames, " - "so this is a failure of the machine and not of the " - "data - free the resource it needs (a GPU shared with " - "another process is the usual cause) and run again.", - *best.indexer_error)); + if (!best.result.has_value()) throw NoLatticeFound(std::string("Two-pass rotation indexing found no lattice.") + next_steps); - } } // Whether the run has a lattice at all is decided here, on the pooled spots rather than on diff --git a/rugnux/RugnuxImagePass.cpp b/rugnux/RugnuxImagePass.cpp index 05e24de27..caa246819 100644 --- a/rugnux/RugnuxImagePass.cpp +++ b/rugnux/RugnuxImagePass.cpp @@ -269,6 +269,9 @@ void Rugnux::ImagePass(PipelineLocals &p) { logger.Error("Error analyzing image {}: {}", image_idx, e.what()); continue; } + // An indexer that failed left this frame unindexed without looking at it; the run stops + // rather than merge what the machine dropped. + indexer->RethrowIndexerError(); pinned.reset(); msg.processing_time_s = std::chrono::duration(std::chrono::steady_clock::now() - t0).count(); msg.run_number = experiment_.GetRunNumber(); @@ -571,7 +574,9 @@ void Rugnux::ImagePass(PipelineLocals &p) { end_msg.indexing_rate = result.indexing_rate; if (full && !cancelled_) { - if (const auto rot = indexer->FinalizeRotationIndexing(); rot.has_value()) { + const auto rot = indexer->FinalizeRotationIndexing(); + indexer->RethrowIndexerError(); + if (rot.has_value()) { last_rotation_result_ = *rot; end_msg.rotation_lattice = rot->lattice; // The refined geometry, not the nominal StartMessage values. Two things about these five diff --git a/rugnux/RugnuxPasses.cpp b/rugnux/RugnuxPasses.cpp index 16836812b..19f87c98f 100644 --- a/rugnux/RugnuxPasses.cpp +++ b/rugnux/RugnuxPasses.cpp @@ -441,7 +441,9 @@ ProcessResult Rugnux::RunAllPasses(RugnuxObserver *observer) { } catch (const std::exception &e) { // A hypothesis that does not survive its own pass is not a failure of the run: the // file's centre has already produced a complete first pass, and this arm exists only - // to be compared against it. + // to be compared against it. A machine that ran out of a resource is, though: the + // file's centre would meet it next. + if (IsFatalResourceError(e)) throw; logger.Warning("Beam centre check: the pass at the measured centre did not complete " "({}) - the run keeps the file's centre", e.what()); } diff --git a/rugnux/RugnuxScaleMerge.cpp b/rugnux/RugnuxScaleMerge.cpp index 1b6dd982f..6fd346d02 100644 --- a/rugnux/RugnuxScaleMerge.cpp +++ b/rugnux/RugnuxScaleMerge.cpp @@ -749,7 +749,9 @@ bool Rugnux::ScaleMergeAndSymmetry(PipelineLocals &p) { } } if (result.consensus_cell.has_value()) { - if (const auto rot = indexer->FinalizeRotationIndexing(); rot && rot->axis) { + const auto rot = indexer->FinalizeRotationIndexing(); + indexer->RethrowIndexerError(); + if (rot && rot->axis) { if (geometry_prepass) prepass_result_ = *rot; // full result (lattice+geom+axis), for the supercell fallback PostRefineSettings prs; diff --git a/rugnux/RugnuxStills.cpp b/rugnux/RugnuxStills.cpp index e2e6ab916..2c938876d 100644 --- a/rugnux/RugnuxStills.cpp +++ b/rugnux/RugnuxStills.cpp @@ -191,6 +191,7 @@ void Rugnux::RefineStillsGeometry(int start_image, int end_image, int images_to_ if (IsFatalResourceError(e)) throw; continue; } + indexer.RethrowIndexerError(); // a frame the indexer failed on is not one that did not index if (!msg.indexing_result.value_or(false) || !msg.indexing_lattice.has_value()) continue; diff --git a/tests/CudaHandledErrorTest.cpp b/tests/CudaHandledErrorTest.cpp index 9889ddfc1..101dd9f0d 100644 --- a/tests/CudaHandledErrorTest.cpp +++ b/tests/CudaHandledErrorTest.cpp @@ -29,7 +29,9 @@ TEST_CASE("CudaDevicePtr_FailedAllocationLeavesErrorUntilCleared", "[CUDAMemHelp SKIP("No CUDA GPU present"); cuda_clear_error(); - REQUIRE_THROWS(CudaDevicePtr(std::numeric_limits::max(), CudaAlloc::Synchronous)); + // Said as what it is, with the remedy, not as a bare CUDA status. + REQUIRE_THROWS_WITH(CudaDevicePtr(std::numeric_limits::max(), CudaAlloc::Synchronous), + Catch::Matchers::ContainsSubstring("another program is using this GPU")); // Work that goes fine in between changes nothing: the error is still waiting. CudaDevicePtr fine(1 << 16); diff --git a/tests/RotationIndexerTest.cpp b/tests/RotationIndexerTest.cpp index a98f74fe6..5bf029ed6 100644 --- a/tests/RotationIndexerTest.cpp +++ b/tests/RotationIndexerTest.cpp @@ -75,9 +75,6 @@ TEST_CASE("RotationIndexer") { } CHECK(cnt == 20); - // An indexer that ran records no error; only one that threw does. A caller reporting "no lattice" - // to the user tells the two apart on this. - CHECK_FALSE(indexer.GetIndexerError().has_value()); auto ret = indexer.GetLattice(); REQUIRE(ret.has_value()); From eb1558e7e2aafe59f9c663367215acbaf63e7d42 Mon Sep 17 00:00:00 2001 From: Filip Leonarski Date: Sat, 10 Oct 2026 18:56:07 +0200 Subject: [PATCH 2/4] rugnux first pass: "no lattice" is never printed or compared as a frame count A pass that finds no lattice carries score -1. That sentinel was printed as "-1/60 validation frames" and compared as a count: the axis-sign rescue adopted the opposite sign when it scored 0 frames against the file's "no lattice" (0 > -1), and the long-axis rescue could return a re-index that indexed nothing over a standing pass that found nothing. Both rescues now count a missing lattice as 0 frames, so an alternative needs at least one indexed frame to win. The log says "no lattice" (Frames()) in the axis-sign, beam-centre, long-axis and rung lines and in the near-tie lines, whose margins also count it as 0 (log only). On the base binary's full open+inhouse battery no set adopted a sign or a long-axis re-index against a lattice-less pass, so no result moves. Co-Authored-By: Claude Opus 5.5 (1M context) --- rugnux/RugnuxFirstPass.cpp | 18 +++--- rugnux/RugnuxFirstPass.h | 2 + rugnux/RugnuxFirstPassRescues.cpp | 103 ++++++++++++++---------------- 3 files changed, 59 insertions(+), 64 deletions(-) diff --git a/rugnux/RugnuxFirstPass.cpp b/rugnux/RugnuxFirstPass.cpp index 8aa476a60..1b75b11c9 100644 --- a/rugnux/RugnuxFirstPass.cpp +++ b/rugnux/RugnuxFirstPass.cpp @@ -1151,14 +1151,13 @@ auto Rugnux::FirstPassRun::FirstPassLadder(int score_before, int min_gain, int m for (size_t i = 0; i < rungs.size() && !rx.cancelled_; i++) { enter_rung(i); FirstPass alt = ScoreSchemes(rung_ris[i], *indexer); - logger.Info(" first-pass rung: {} spots/image, rings {}, stop at {} -> {}/{} frames{}", + logger.Info(" first-pass rung: {} spots/image, rings {}, stop at {} -> {}{}", rungs[i].seed == std::numeric_limits::max() ? std::string("all") : std::to_string(rungs[i].seed), rungs[i].rings ? "aside" : "kept", rungs[i].d_min ? fmt::format("{:.2f} A", *rungs[i].d_min) : std::string("the detector edge"), - alt.result.has_value() ? alt.score : -1, - static_cast(validation.size()), + alt.result.has_value() ? Frames(alt.score) + " frames" : Frames(-1), alt.result.has_value() ? fmt::format(", {}{} {:.1f} {:.1f} {:.1f} {:.1f} {:.1f} {:.1f}", alt.result->search_result.centering, @@ -1598,6 +1597,10 @@ void Rugnux::FirstPassRun::NearTie(const std::string &decision, const std::strin decision, a, b, margin, noise, model, PassTag()); } +std::string Rugnux::FirstPassRun::Frames(int score) const { + return score < 0 ? std::string("no lattice") : fmt::format("{}/{}", score, validation.size()); +} + void Rugnux::FirstPassRun::NearTieFrames(const std::string &decision, const std::string &a, int ka, const std::string &b, int kb) const { // Two lattices that both index fewer frames than the bar the run keeps a lattice on (a sixth of @@ -1609,17 +1612,16 @@ void Rugnux::FirstPassRun::NearTieFrames(const std::string &decision, const std: const double p = (std::max(k, 0) + 0.5) / (n + 1.0); return n * p * (1.0 - p); }; - NearTie(decision, fmt::format("{} {}/{}", a, ka, validation.size()), - fmt::format("{} {}/{}", b, kb, validation.size()), - ka - kb, std::sqrt(var(ka) + var(kb)), "binomial on validation frames"); + NearTie(decision, fmt::format("{} {}", a, Frames(ka)), fmt::format("{} {}", b, Frames(kb)), + std::max(ka, 0) - std::max(kb, 0), std::sqrt(var(ka) + var(kb)), "binomial on validation frames"); } void Rugnux::FirstPassRun::NearTieGate(const std::string &decision, const std::string &what, int score, double gate) const { const double n = static_cast(validation.size()); const double p = (std::max(score, 0) + 0.5) / (n + 1.0); - NearTie(decision, fmt::format("{} {}/{}", what, score, validation.size()), - fmt::format("gate {:.1f}", gate), score - gate, std::sqrt(n * p * (1.0 - p)), + NearTie(decision, fmt::format("{} {}", what, Frames(score)), + fmt::format("gate {:.1f}", gate), std::max(score, 0) - gate, std::sqrt(n * p * (1.0 - p)), "binomial on validation frames"); } diff --git a/rugnux/RugnuxFirstPass.h b/rugnux/RugnuxFirstPass.h index d9f6cc7c0..b91870ff0 100644 --- a/rugnux/RugnuxFirstPass.h +++ b/rugnux/RugnuxFirstPass.h @@ -150,6 +150,8 @@ struct Rugnux::FirstPassRun { // and for two pooled spot shares the binomial standard error ValidationEvidencePrefers uses. static constexpr double NEAR_TIE_SIGMA = 2.0; std::string PassTag() const; + // A frame count as the log gives it: "k/N", or "no lattice" for a pass that found none. + std::string Frames(int score) const; void NearTie(const std::string &decision, const std::string &a, const std::string &b, double margin, double noise, const char *model) const; // Two frame counts, a against b; nothing where neither reaches the run's floor of a sixth. diff --git a/rugnux/RugnuxFirstPassRescues.cpp b/rugnux/RugnuxFirstPassRescues.cpp index 4863f6116..3c1d8f8cc 100644 --- a/rugnux/RugnuxFirstPassRescues.cpp +++ b/rugnux/RugnuxFirstPassRescues.cpp @@ -130,21 +130,21 @@ auto Rugnux::FirstPassRun::AxisSignRescue(FirstPass best) -> FirstPass { // the flipped lattice must also beat chance on the pooled spots - the test the run puts // its final lattice to - which a sparse pattern at the right sign passes on few frames // (2/60 frames, 89% of the validation spots against 0% by chance). - const bool alt_real = alt.result.has_value() && alt.score > best.score - && ValidationEvidenceBeatsChance(MeasurePooled(*indexer, *alt.result)); + // A file's sign that found no lattice counts as 0 frames: no answer is not a count the + // opposite sign can beat with another 0. + const bool more_frames = alt.result.has_value() && alt.score > std::max(best.score, 0); + const bool alt_real = more_frames && ValidationEvidenceBeatsChance(MeasurePooled(*indexer, *alt.result)); if (alt_real) { - logger.Info("Rotation axis sign: the file's axis indexes {}/{} validation frames, " - "the opposite sign {}/{} - adopting the opposite sign for this run", - best.score, static_cast(validation.size()), - alt.score, static_cast(validation.size())); + logger.Info("Rotation axis sign: validation frames indexed at the file's axis {}, at the " + "opposite sign {} - adopting the opposite sign for this run", + Frames(best.score), Frames(alt.score)); best = alt; } else { - if (alt.result.has_value() && alt.score > best.score) - logger.Info("Rotation axis sign: the opposite sign indexes {}/{} validation frames " - "against the file's {}/{}, but its lattice does not beat chance on the " - "pooled spots - the file's sign is kept", - alt.score, static_cast(validation.size()), - best.score, static_cast(validation.size())); + if (more_frames) + logger.Info("Rotation axis sign: validation frames indexed at the opposite sign {}, at " + "the file's axis {}, but its lattice does not beat chance on the pooled " + "spots - the file's sign is kept", + Frames(alt.score), Frames(best.score)); rx.experiment_.Goniometer(*gon); // the file was right; put it back } } @@ -221,7 +221,7 @@ auto Rugnux::FirstPassRun::LongAxisRescue(const FirstPass &standing) -> std::opt rx.experiment_.SetUnitCell(std::nullopt); // leave the space-group / cell determination de-novo if (constrained.result.has_value()) NearTieFrames("long-axis rescue", "re-index", constrained.score, "standing", standing.score); - if (constrained.result.has_value() && constrained.score > standing.score) + if (constrained.result.has_value() && constrained.score > std::max(standing.score, 0)) return constrained; } return std::nullopt; @@ -296,10 +296,8 @@ auto Rugnux::FirstPassRun::BeamCentreCheck(FirstPass best, bool &long_axis_asked if (measured_indexes) return CentreWhenBothIndex(std::move(best), alt, c); logger.Info("Beam centre check: the measured centre ({:.2f},{:.2f}) indexes " - "{}/{} validation frames against the file's {}/{} - the file's " - "centre stands", measured_x, measured_y, alt.score, - static_cast(validation.size()), best.score, - static_cast(validation.size())); + "{} validation frames against the file's {} - the file's " + "centre stands", measured_x, measured_y, Frames(alt.score), Frames(best.score)); return best; } @@ -355,15 +353,13 @@ auto Rugnux::FirstPassRun::CentreWhenFileFails(FirstPass best, FirstPass alt, co const FirstPass both = PickBest(*indexer_pool, *indexer); if (both.result.has_value() && both.score > majority) { logger.Warning("Beam centre from the background: the file's centre " - "({:.2f},{:.2f}) indexes {}/{} validation frames and the " - "measured ({:.2f},{:.2f}) {}/{}, but the measured centre " - "WITH the opposite rotation-axis sign indexes {}/{}. The " + "({:.2f},{:.2f}) indexes {} validation frames and the " + "measured ({:.2f},{:.2f}) {}, but the measured centre " + "WITH the opposite rotation-axis sign indexes {}. The " "file has both wrong, and neither shows while the other " "stands; the run takes the measured centre and the opposite " - "sign", header_x, header_y, best.score, - static_cast(validation.size()), measured_x, measured_y, - alt.score, static_cast(validation.size()), both.score, - static_cast(validation.size())); + "sign", header_x, header_y, Frames(best.score), measured_x, measured_y, + Frames(alt.score), Frames(both.score)); alt = both; measured_indexes = true; joint = true; @@ -427,21 +423,20 @@ auto Rugnux::FirstPassRun::CentreWhenFileFails(FirstPass best, FirstPass alt, co rx.beam_center_alternative_ = std::array{measured_x, measured_y}; rx.beam_center_alternative_state_ = away_state; logger.Warning("Beam centre check: reading less of each frame, the " - "file's centre indexes {}/{} validation frames and the " - "measured centre {:.2f} px away {}/{}, on the same " + "file's centre indexes {} validation frames and the " + "measured centre {:.2f} px away {}, on the same " "lattice - the counts cannot say which centre is right, " "so the run carries BOTH centres forward and adopts " "whichever measures more of the crystal", - home->score, static_cast(validation.size()), moved, - away->score, static_cast(validation.size())); + Frames(home->score), moved, + Frames(away->score)); } else logger.Warning("Beam centre check: reading less of each frame takes the " - "file's centre to {}/{} validation frames against {}/{} at " + "file's centre to {} validation frames against {} at " "the measured centre {:.2f} px away - it is how deep this " "run was reading and not the centre, so the centre stays", - home->score, static_cast(validation.size()), - away ? away->score : 0, - static_cast(validation.size()), moved); + Frames(home->score), + Frames(away ? away->score : 0), moved); } else if (away) { alt = *away; measured_indexes = true; @@ -452,20 +447,17 @@ auto Rugnux::FirstPassRun::CentreWhenFileFails(FirstPass best, FirstPass alt, co if (lean) logger.Warning("Beam centre from the background: neither centre indexes the " "whole spot list, but at the measured ({:.2f},{:.2f}) the " - "lean first pass above indexes {}/{} against the file's " - "{}/{} - the beam centre in this file is wrong, and only a " + "lean first pass above indexes {} against the file's " + "{} - the beam centre in this file is wrong, and only a " "pass that reads less of each frame could see it", - measured_x, measured_y, alt.score, - static_cast(validation.size()), best.score, - static_cast(validation.size())); + measured_x, measured_y, Frames(alt.score), Frames(best.score)); else if (!joint) logger.Warning("Beam centre from the background: the file's ({:.2f},{:.2f}) " - "indexes {}/{} validation frames and the measured " - "({:.2f},{:.2f}) indexes {}/{} - the beam centre in this " + "indexes {} validation frames and the measured " + "({:.2f},{:.2f}) indexes {} - the beam centre in this " "file is wrong, and the run adopts the measured one", - header_x, header_y, best.score, - static_cast(validation.size()), measured_x, measured_y, - alt.score, static_cast(validation.size())); + header_x, header_y, Frames(best.score), measured_x, measured_y, + Frames(alt.score)); best = alt; } else if (!deeper_at_home) { best = CentreByPooledSpots(std::move(best), std::move(alt), c, long_axis_asked); @@ -513,13 +505,13 @@ auto Rugnux::FirstPassRun::CentreByPooledSpots(FirstPass best, FirstPass alt, co && ValidationEvidencePrefers(at_header, at_measured)) { TryBeamCenter(measured_x, measured_y); logger.Warning("Beam centre from the background: neither centre indexes a " - "majority of the validation frames ({}/{} at the measured, " - "{}/{} at the file's), but the pooled spots put " + "majority of the validation frames ({} at the measured, " + "{} at the file's), but the pooled spots put " "{:.1f}% on the lattice at the measured ({:.2f},{:.2f}) against " "{:.1f}% at the file's (chance {:.1f}% and {:.1f}%) - the run " "adopts the measured centre", - alt.score, static_cast(validation.size()), - best.score, static_cast(validation.size()), + Frames(alt.score), + Frames(best.score), 100.0 * at_measured.on_lattice / std::max(1, at_measured.spots), measured_x, measured_y, 100.0 * at_header.on_lattice / std::max(1, at_header.spots), @@ -527,12 +519,11 @@ auto Rugnux::FirstPassRun::CentreByPooledSpots(FirstPass best, FirstPass alt, co 100.0 * at_header.by_chance / std::max(1, at_header.spots)); best = alt; } else { - logger.Info("Beam centre check: the file's centre ({}/{} frames) and the " - "measured one {:.2f} px away ({}/{}) - the measured centre puts no " + logger.Info("Beam centre check: the file's centre ({} frames) and the " + "measured one {:.2f} px away ({}) - the measured centre puts no " "more of the pooled spots on its lattice, so the file's centre is " "kept", - best.score, static_cast(validation.size()), moved, alt.score, - static_cast(validation.size())); + Frames(best.score), moved, Frames(alt.score)); } return best; } @@ -643,17 +634,17 @@ auto Rugnux::FirstPassRun::CentresDisagree(FirstPass best, const FirstPass &alt, logger.Warning("Beam centre check: the two centres DISAGREE about the " "lattice. The file's ({:.2f},{:.2f}) gives {}-centred {}, " "{:.2f} {:.2f} {:.2f} {:.2f} {:.2f} {:.2f} (primitive volume " - "{:.0f} A^3, {}/{} frames); the measured ({:.2f},{:.2f}) gives " + "{:.0f} A^3, {} frames); the measured ({:.2f},{:.2f}) gives " "{}-centred {}, {:.2f} {:.2f} {:.2f} {:.2f} {:.2f} {:.2f} " - "(primitive volume {:.0f} A^3, {}/{} frames){}", + "(primitive volume {:.0f} A^3, {} frames){}", header_x, header_y, best.result->search_result.centering, gemmi::crystal_system_str(best.result->search_result.system), hc.a, hc.b, hc.c, hc.alpha, hc.beta, hc.gamma, best.vol, - best.score, static_cast(validation.size()), + Frames(best.score), measured_x, measured_y, alt.result->search_result.centering, gemmi::crystal_system_str(alt.result->search_result.system), mc.a, mc.b, mc.c, mc.alpha, mc.beta, mc.gamma, alt.vol, - alt.score, static_cast(validation.size()), + Frames(alt.score), harmonic ? fmt::format(" - the volumes differ by {:.2f}x, so one " "is an axis harmonic of the other and the " "beam centre is what settles which", @@ -965,9 +956,9 @@ auto Rugnux::FirstPassRun::SubLatticeHypothesis(FirstPass best) -> FirstPass { at_standing.on_lattice, at_standing.spots, at_standing.by_chance, adopt ? "adopting it" : "keeping the standing cell"); } - logger.Info("Sub-lattice hypothesis: the standing cell ({:.0f} A^3) indexes {}/{} validation " + logger.Info("Sub-lattice hypothesis: the standing cell ({:.0f} A^3) indexes {} validation " "frames; its index-2 sub-lattices (half-translation u,v,w):{}{}", - best.vol, best.score, static_cast(validation.size()), counts, verdict); + best.vol, Frames(best.score), counts, verdict); if (adopt) best = std::move(*sub_best); } From c9cb3c470346f651b368f11832f316693d2e6bca Mon Sep 17 00:00:00 2001 From: Filip Leonarski Date: Sat, 10 Oct 2026 18:56:18 +0200 Subject: [PATCH 3/4] rugnux: a beam-centre arm that loses leaves no trace on the run When the first pass is run again at the measured beam centre and the file's centre is kept, the run restored the experiment, spot finding, mosaicity, geometry and prepass result, but not the rest of what the arm's pass writes: its reflection footprint replaced bragg_adaptive_ (the integration settings of the canonical pass), and last_rotation_result_, bkg_starved_fraction_, bragg_before_adaptive_ and supercell_settled_ stayed the arm's. A losing arm therefore changed the result, and an arm that completed gave a different answer from one that failed: on an open-arm tetragonal set 66132699 against 66121594 integrated reflections and a different p.hkl. These are now restored with the rest. Verified on that set with a forced arm failure (temporary local patch, not committed): the arm completing and the arm failing now give the same p.hkl (99faeb99), which is also what the arm failing gave before (stress runs #0/#1); the base binary gave 94c3afc9 when the arm completed. Co-Authored-By: Claude Opus 5.5 (1M context) --- rugnux/RugnuxPasses.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/rugnux/RugnuxPasses.cpp b/rugnux/RugnuxPasses.cpp index 19f87c98f..4c10582ec 100644 --- a/rugnux/RugnuxPasses.cpp +++ b/rugnux/RugnuxPasses.cpp @@ -423,6 +423,14 @@ ProcessResult Rugnux::RunAllPasses(RugnuxObserver *observer) { const auto file_mosaicity = prepass_mosaicity_; const auto file_geometry = prepass_detector_geometry_; const auto file_result = prepass_result_; + // What else a pass leaves on the run that a later pass reads. The arm's own reflection + // footprint replaced the canonical integration settings and its last rotation result the + // doubled-cell pass's starting point, so an arm that lost still moved the result. + const auto file_bragg_adaptive = bragg_adaptive_; + const auto file_bragg_before_adaptive = bragg_before_adaptive_; + const auto file_bkg_starved_fraction = bkg_starved_fraction_; + const auto file_last_rotation_result = last_rotation_result_; + const bool file_supercell_settled = supercell_settled_; experiment_ = experiment_before_first_pass; // The measured spot width is a property of the crystal and was measured once, before this @@ -504,6 +512,11 @@ ProcessResult Rugnux::RunAllPasses(RugnuxObserver *observer) { prepass_mosaicity_ = file_mosaicity; prepass_detector_geometry_ = file_geometry; prepass_result_ = file_result; + bragg_adaptive_ = file_bragg_adaptive; + bragg_before_adaptive_ = file_bragg_before_adaptive; + bkg_starved_fraction_ = file_bkg_starved_fraction; + last_rotation_result_ = file_last_rotation_result; + supercell_settled_ = file_supercell_settled; } // The arm's own pass asks the same question again at its own centre; the run has already // decided it, so nothing is to consume a second answer. From 89a7853916dfa1f75c62686725ee23c2455b8271 Mon Sep 17 00:00:00 2001 From: Filip Leonarski Date: Sat, 10 Oct 2026 19:34:51 +0200 Subject: [PATCH 4/4] A failed cuFFT plan gets the GPU-memory advice whatever its code In a stress run (two rugnux on one 16 GB card) the indexer's cufftPlanMany failed three times with CUFFT_INTERNAL_ERROR (5), not CUFFT_ALLOC_FAILED, so the message carried neither the free memory nor the remedy. Every plan failure is now reported like a failed allocation: free memory on the card, the two usual causes and the CPU remedy. Co-Authored-By: Claude Opus 5.5 (1M context) --- image_analysis/indexing/CUDAMemHelpers.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/image_analysis/indexing/CUDAMemHelpers.h b/image_analysis/indexing/CUDAMemHelpers.h index 8945c0fc1..6611beb05 100644 --- a/image_analysis/indexing/CUDAMemHelpers.h +++ b/image_analysis/indexing/CUDAMemHelpers.h @@ -122,10 +122,11 @@ public: const_cast(inembed), istride, idist, const_cast(onembed), ostride, odist, type, batch); + // Said as a memory failure whatever the code: on a full card (two runs sharing it) the plan + // failed as CUFFT_INTERNAL_ERROR, not CUFFT_ALLOC_FAILED. if (r != CUFFT_SUCCESS) throw JFJochException(JFJochExceptionCategory::GPUCUDAError, - GPUMemoryFailure("Creating the CUFFT plan (cufftPlanMany)", 0, - r == CUFFT_ALLOC_FAILED, + GPUMemoryFailure("Creating the CUFFT plan (cufftPlanMany)", 0, true, ("cuFFT error " + std::to_string(r)).c_str())); }