diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index a7718a4b2..4b02e4bc1 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -3,6 +3,7 @@ ### 1.0.0-rc.170 +* Fixed a `jfjoch_broker` crash during indexing: a non-finite value reaching the FFT shortlist, space-group operator ranking or indexing-refinement sorts no longer causes undefined behaviour, and GPU FFT indexer kernel launches are now error-checked. * rugnux needs about a third less peak memory to scale, merge and post-refine rotation data with tens of millions of partial observations, with identical results. * `rugnux --model`: the placed coordinate file carries the space group its own coordinates obey, and says so when that is not the group the reflection files beside it carry. * `jfjoch_viewer`: fixes in the dataset plots (the stacked plots line up their y axes, the horizontal crosshair follows the cursor, a run in progress labels the angles of its whole sweep), in the inspector (a long dataset name wraps instead of being cut off, a live session shows its sample-head angles) and in the layout (the resize handle beside the file panel is centred). diff --git a/image_analysis/indexing/FFTIndexer.cpp b/image_analysis/indexing/FFTIndexer.cpp index 7bec2f92d..08fb35f77 100644 --- a/image_analysis/indexing/FFTIndexer.cpp +++ b/image_analysis/indexing/FFTIndexer.cpp @@ -2,6 +2,7 @@ // SPDX-License-Identifier: GPL-3.0-only #include +#include #include #include "../../common/JFJochMath.h" #include "FFTIndexer.h" @@ -185,8 +186,14 @@ std::vector FFTIndexer::FilterFFTResults(size_t max_vectors, std::vector *magnitudes) const { std::multimap fft_result_map; - for (int i = 0; i < direction_vectors.size(); i++) + // No legitimate row is non-finite (lengths are bin*coeff or -1, magnitudes guarded + // prominences), but a corrupted result buffer would put NaN into the length sort below, + // and std::sort with a NaN key walks out of bounds. Drop such rows instead. + for (int i = 0; i < direction_vectors.size(); i++) { + if (!std::isfinite(result_fft[i].magnitude) || !std::isfinite(result_fft[i].length)) + continue; fft_result_map.insert(std::make_pair(result_fft[i].magnitude, result_fft[i])); + } std::vector fft_result_filtered; int count = 0; diff --git a/image_analysis/indexing/FFTIndexerGPU.cu b/image_analysis/indexing/FFTIndexerGPU.cu index 343446845..ce0959a90 100644 --- a/image_analysis/indexing/FFTIndexerGPU.cu +++ b/image_analysis/indexing/FFTIndexerGPU.cu @@ -226,6 +226,7 @@ void FFTIndexerGPU::ExecuteFFT(const std::vector &coord, size_t nspots) { direction_vectors.size(), d_input_fft); } + cuda_err(cudaGetLastError()); cuda_err(cufftExecR2C(plan, d_input_fft, d_output_fft)); @@ -238,6 +239,7 @@ void FFTIndexerGPU::ExecuteFFT(const std::vector &coord, size_t nspots) { max_length_A, min_length_A, histogram_size, bg_half, direction_vectors.size(), d_result_fft); + cuda_err(cudaGetLastError()); cuda_err(cudaMemcpyAsync(result_fft.data(), d_result_fft, direction_vectors.size() * sizeof(FFTResult), cudaMemcpyDeviceToHost, stream)); diff --git a/image_analysis/indexing/PostIndexingRefinement.cpp b/image_analysis/indexing/PostIndexingRefinement.cpp index e3f20dc3c..cd03b819d 100644 --- a/image_analysis/indexing/PostIndexingRefinement.cpp +++ b/image_analysis/indexing/PostIndexingRefinement.cpp @@ -5,6 +5,7 @@ #include "PostIndexingRefinement.h" #include +#include #include namespace { @@ -118,6 +119,10 @@ namespace { resid = CalculateResiduals(spots, cell); ArrayX dist = resid.rowwise().norm(); + // A singular refined cell puts inf into cell.inverse() and 0*inf = NaN into the + // residuals; a NaN key is UB in nth_element. Such a spot does not index at all, + // which is exactly what +inf says - and inf, unlike NaN, orders consistently. + dist = dist.isFinite().select(dist, std::numeric_limits::infinity()); auto nth = std::begin(dist) + (cifssr.min_spots - 1); std::nth_element(std::begin(dist), nth, std::end(dist)); scores(j) = *nth; diff --git a/image_analysis/scale_merge/SearchSpaceGroup.cpp b/image_analysis/scale_merge/SearchSpaceGroup.cpp index a54a4603e..d220ae05c 100644 --- a/image_analysis/scale_merge/SearchSpaceGroup.cpp +++ b/image_analysis/scale_merge/SearchSpaceGroup.cpp @@ -1619,7 +1619,13 @@ SearchSpaceGroupResult SearchSpaceGroup( for (const auto& [rk, s] : op_cache) result.operator_scores.push_back(s); std::sort(result.operator_scores.begin(), result.operator_scores.end(), - [](const auto& a, const auto& b) { return a.cc > b.cc; }); + [](const auto& a, const auto& b) { + // cc is NaN for an operator PearsonCC could not score; a NaN key breaks + // std::sort's ordering (out-of-bounds UB), so rank those last explicitly. + if (std::isfinite(a.cc) != std::isfinite(b.cc)) + return std::isfinite(a.cc); + return a.cc > b.cc; + }); // A caller that already decided the point group elsewhere overrides the choice here, keeping the // operator scores and the refusal report Stage A just produced. Nothing else is bypassed: Stage B diff --git a/rugnux/ModelScaling.cpp b/rugnux/ModelScaling.cpp index 600b7df3f..9a35b929d 100644 --- a/rugnux/ModelScaling.cpp +++ b/rugnux/ModelScaling.cpp @@ -54,7 +54,8 @@ ModelScaleReport FitModelScale(gemmi::Scaling &scaling, ModelScaleBox box scaling.fit_parameters(); // k_overall + anisotropic B only ++report.n_grid; const double r = RFactor(scaling); - if (best_r < 0 || r < best_r) { + // A diverged fit gives r = NaN; latched as best_r it wins every later r < best_r. + if (std::isfinite(r) && (best_r < 0 || r < best_r)) { best_r = r; best_k_sol = k_sol; best_b_sol = b_sol;