From 6319d5c600e1f87235833f91f8a8caa60f14bda2 Mon Sep 17 00:00:00 2001 From: Filip Leonarski Date: Sat, 29 Aug 2026 15:30:28 +0200 Subject: [PATCH] indexing: tell an indexer that failed apart from one that found nothing IndexerThreadPool collapsed two different outcomes into the same empty reply. A worker that threw set result = nullptr, and so did a dispatch that never found a free worker; both then became a default-constructed IndexerResult, indistinguishable downstream from an indexer that ran and found no lattice. The first says nothing about the frame at all - the indexer never looked, and it will fail again on the next one - while the second is a real negative result about the crystal. The visible cost was the advice a failed run gave. With the card full, rugnux printed "Indexer thread 0 failed: CUDA (GPU) error" and then ended with "Two-pass rotation indexing found no lattice. Check the beam centre (--beam-x / --beam-y), raise --max-spots ..." - sending the operator to look at geometry that was never wrong, for a machine that was simply out of memory. None of those remedies can help when the frames were not examined. IndexerResult gains an optional error, set by the worker and by the pool's own catch; the remaining nullptr path keeps its meaning of "not attempted" and deliberately carries no error. RotationIndexer records it and exposes GetIndexerError(), and rugnux's first pass branches on it at the throw site, naming the resource failure instead. The error travels as DATA through image_analysis/ rather than as an exception, because RotationIndexer::RunIndexing() is on the online path - IndexAndRefine calls it on a schedule from the broker and the receiver, where dropping a frame is the right failure and killing a live acquisition is not. Only rugnux, which owns the "this run is over" decision, turns it into one. The failure result is byte-identical to the default-constructed one it replaces, so any_executed and the online frame-drop behaviour are unchanged. The new IndexerError category exists because the category is only the display prefix on what() - Category() is read nowhere - and the old line read "Processing failed: Input parameter invalid" for a GPU fault, which is the same defect one layer up. SpotFinderError is the precedent. msg.indexing_result is deliberately left alone: of its three states, "not attempted" is the honest one for a frame the indexer never examined, and asserting false would be the same collapse again. Verified on a squeezed card (246 MB free, -N 4): exit 1, zero dropped frames, the new message, and no mention of --beam-x. On a free card, against the previous binary at -N 6 on the same input, the logs differ only in paths and timing - same space group, cell and merge statistics. Targeted Catch2 cases pass, including three that drive the pool through the online path. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Lc5JG6kJqZoCWaoZ43JGTW --- common/JFJochException.h | 5 +++- docs/CHANGELOG.md | 1 + image_analysis/indexing/Indexer.h | 5 ++++ image_analysis/indexing/IndexerThreadPool.cpp | 11 ++++++-- .../rotation_indexer/RotationIndexer.cpp | 6 +++++ .../rotation_indexer/RotationIndexer.h | 5 ++++ rugnux/Rugnux.cpp | 27 +++++++++++++++---- tests/RotationIndexerTest.cpp | 3 +++ 8 files changed, 55 insertions(+), 8 deletions(-) diff --git a/common/JFJochException.h b/common/JFJochException.h index 5a13b5bec..fa3c64809 100644 --- a/common/JFJochException.h +++ b/common/JFJochException.h @@ -44,7 +44,8 @@ CBORError, UDPError, CommunicationError, SimplonError, -CalibrationError +CalibrationError, +IndexerError }; class JFJochException : public std::exception { @@ -125,6 +126,8 @@ protected: return "Simplon API error"; case JFJochExceptionCategory::CalibrationError: return "Detector calibration error"; + case JFJochExceptionCategory::IndexerError: + return "Indexer error"; default: return ""; } diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 65d7adfd6..1625f4509 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -1,6 +1,7 @@ # Changelog ## 1.0.0 ### 1.0.0-rc.166 +* A run whose indexer could not run at all - an exhausted GPU, most often - reports that, instead of reporting that no lattice was found and advising the beam centre be checked. * A run that exhausts GPU or host memory stops with an error instead of skipping the images it could not process and finishing as if they had never been there. * Each scaling pass of a de-novo rotation run runs the number of scaling iterations it was asked for, instead of continuing the previous pass's. A run given its space group with `-S` is unaffected. * A point-group promotion that fails the merge chi-squared test needs a smaller rise in its merge's systematic error to be rescued from it, so a merohedrally twinned crystal is kept in its true lower symmetry. diff --git a/image_analysis/indexing/Indexer.h b/image_analysis/indexing/Indexer.h index dc2dc7835..26b7766e3 100644 --- a/image_analysis/indexing/Indexer.h +++ b/image_analysis/indexing/Indexer.h @@ -15,6 +15,11 @@ 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; }; class Indexer { diff --git a/image_analysis/indexing/IndexerThreadPool.cpp b/image_analysis/indexing/IndexerThreadPool.cpp index 30e6952ee..8684c3307 100644 --- a/image_analysis/indexing/IndexerThreadPool.cpp +++ b/image_analysis/indexing/IndexerThreadPool.cpp @@ -150,8 +150,12 @@ void IndexerThread::Worker(int threadid) { indexer.Setup(input->experiment); tmp_result = std::make_unique(indexer.Run(input->recip)); } catch (std::exception &e) { - tmp_result = nullptr; + // Hand the failure back as a result carrying the reason. A nullptr here was + // indistinguishable from a worker that was never dispatched, and both then read + // 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()}); } { std::unique_lock lock(m); @@ -284,7 +288,8 @@ IndexerResult IndexerThreadPool::Run(const DiffractionExperiment &experiment, co result = tasks[task]->Run(experiment, recip); } catch (const std::exception &e) { spdlog::error("Indexer thread failed: {}", e.what()); - result = nullptr; + result = std::make_unique(IndexerResult{ + .lattice = {}, .indexing_time_s = 0, .executed = false, .error = e.what()}); } { std::unique_lock lock(m); @@ -295,5 +300,7 @@ IndexerResult IndexerThreadPool::Run(const DiffractionExperiment &experiment, co } if (result) return *result; + // No free worker, or the pool is stopping: indexing was not attempted. Distinct from both a + // frame that did not index and an indexer that failed, and left without an error for that reason. return IndexerResult{.lattice = {}, .indexing_time_s = 0}; } diff --git a/image_analysis/rotation_indexer/RotationIndexer.cpp b/image_analysis/rotation_indexer/RotationIndexer.cpp index 89b8e898a..949ce9052 100644 --- a/image_analysis/rotation_indexer/RotationIndexer.cpp +++ b/image_analysis/rotation_indexer/RotationIndexer.cpp @@ -141,6 +141,7 @@ void RotationIndexer::RunIndexing() { coords.emplace_back(rot * s.ReciprocalCoord(geom_)); } const auto indexer_result = indexer_.Run(experiment, coords); + indexer_error_ = indexer_result.error; if (!indexer_result.lattice.empty() && indexer_result.lattice[0].CalcVolume() > 1.0) { DiffractionExperiment experiment_copy(experiment); @@ -495,6 +496,11 @@ 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 d9b8dd51e..da24336a0 100644 --- a/image_analysis/rotation_indexer/RotationIndexer.h +++ b/image_analysis/rotation_indexer/RotationIndexer.h @@ -53,6 +53,7 @@ class RotationIndexer { size_t accumulated_spots = 0; std::optional indexed_lattice; + std::optional indexer_error_; public: // real_time: bound the candidate-cell refinements by WALL CLOCK, as online acquisition must - it // has a real budget. Offline (rugnux, the viewer) passes false and they are bounded by iteration @@ -65,6 +66,10 @@ public: std::optional angle_deg = std::nullopt); 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/rugnux/Rugnux.cpp b/rugnux/Rugnux.cpp index 9b42681e4..839318b30 100644 --- a/rugnux/Rugnux.cpp +++ b/rugnux/Rugnux.cpp @@ -1798,7 +1798,10 @@ ProcessResult Rugnux::RunPipeline(RugnuxObserver *observer, bool write_output, b {"wedge", all_consecutive}, }; - struct FirstPass { std::optional result; int score = -1; std::string name; double vol = 0.0; }; + // 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. + struct FirstPass { std::optional result; int score = -1; std::string name; double vol = 0.0; + std::optional indexer_error; }; // Feed both schemes (single-threaded spot cache), run their RunIndexing() concurrently, then keep // the lattice that indexes the most validation frames on the real per-image path. @@ -1823,6 +1826,8 @@ ProcessResult Rugnux::RunPipeline(RugnuxObserver *observer, bool write_output, b for (size_t i = 0; i < ris.size(); i++) { if (cancelled_) break; + if (!bp.indexer_error) + bp.indexer_error = ris[i]->GetIndexerError(); auto found = ris[i]->GetLattice(); if (!found.has_value()) continue; @@ -2038,16 +2043,28 @@ ProcessResult Rugnux::RunPipeline(RugnuxObserver *observer, bool write_output, b release_spot_engines(); // no more frames to read; give the cards their memory back if (!cancelled_) { - // The two ways the first pass can end without a usable lattice. They are one situation from - // the user's side - no crystal was found in these frames - so both name the cause and then - // give the same remedies. + // The two ways the first pass can end without a usable lattice that are about the DATA. + // They are one situation from the user's side - no crystal was found in these frames - so + // both name the cause and then give the same remedies. constexpr const char *next_steps = " Nothing was integrated. Check the beam centre (--beam-x / --beam-y), raise " "--max-spots if the frames are well populated with spots, and use --mode azint to " "write the per-image analysis for inspection."; - if (!best.result.has_value()) + 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)); throw JFJochException(JFJochExceptionCategory::InputParameterInvalid, std::string("Two-pass rotation indexing found no lattice.") + next_steps); + } // A lattice that indexes almost none of the validation frames is not this crystal's lattice. // Forcing it anyway integrates every image against it; the few partials that survive yield no // fulls in the 3D combine, and the run then dies inside the merge complaining about resolution - diff --git a/tests/RotationIndexerTest.cpp b/tests/RotationIndexerTest.cpp index dec42359c..8b7b1111f 100644 --- a/tests/RotationIndexerTest.cpp +++ b/tests/RotationIndexerTest.cpp @@ -81,6 +81,9 @@ 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());