From 847e44d71822b1f2a96b04d545377aa48e32dd74 Mon Sep 17 00:00:00 2001 From: Filip Leonarski Date: Wed, 2 Sep 2026 15:22:19 +0200 Subject: [PATCH] indexing: the short-axis pass stays out of a given cell, and four stale comments From the code review of the indexing work. The short-axis pass took its settings from experiment_, which never carries the max bound RaiseFFTBoundForKnownCell raises for a given cell - that runs on a local copy. So with -C naming a cell longer than the default search, the short pool could not represent the cell that was asked for and could only return a sub-multiple of it; a sub-multiple indexes every frame its parent does, which is the premise of this whole comparison, so it would tie and be adopted over the user's own cell. It now skips a run with a known cell entirely, which is also where it had least to offer: the same call has already lowered the FLOOR against that cell for the standard pass, so the two schemes were returning the same lattice and paying two extra FFTs for it. Four comments asserted rules the code no longer follows. The one directly above the decision still said the larger cell is taken as spurious "regardless of scheme order" - the opposite of what the arbiter does - and a dead assignment below it stated the same thing a second time. The added-class enumeration is described as exhaustive; it covers the index-n sublattices with a CYCLIC quotient, which is all of them at n = 2 and n = 3 but not at n = 4, where a supercell doubling two axes has a (Z/2)^2 quotient and reads at the chance value instead. That is a report and decides nothing, but it is quoted in the argument for bounding n at 4, so the bound rests on thinner evidence at n = 4 than it appears to. And std::rint where the identical loop in IndexAndRefine uses it and says why. Co-Authored-By: Claude Opus 5 (1M context) --- rugnux/Rugnux.cpp | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/rugnux/Rugnux.cpp b/rugnux/Rugnux.cpp index 7238b6208..d2a13ff61 100644 --- a/rugnux/Rugnux.cpp +++ b/rugnux/Rugnux.cpp @@ -1821,9 +1821,12 @@ namespace { // // What that added class holds is reported alongside, because it is the physics the count is a // consequence of. Multiplying an axis by n is a CENTRING CONDITION in the larger cell, and the - // index-n sublattices of its Miller lattice are the kernels of the linear forms f = (u,v,w) mod n - // with gcd(u,v,w,n) = 1, so enumerate them and report the emptiest: that is the condition the - // larger cell would have to satisfy to BE its sub-cell. It is reported and NOT thresholded, and + // index-n sublattices of its Miller lattice WITH A CYCLIC QUOTIENT are the kernels of the linear + // forms f = (u,v,w) mod n with gcd(u,v,w,n) = 1, so enumerate them and report the emptiest: at + // n = 2 and n = 3 that is every index-n sublattice, but at n = 4 the seven whose quotient is + // (Z/2)^2 - a supercell doubling TWO axes - are not of that form, so a real 2a x 2b superstructure + // reads at the chance value here. That is the condition the larger cell would have to satisfy to + // BE its sub-cell. It is reported and NOT thresholded, and // that is deliberate. Measured on real data the number does not have two populations to separate: // an absent class reads a few tenths of a percent where nothing but the crystal is in the spot // list and several percent where the finder also picked up things that are not this crystal's @@ -1841,7 +1844,7 @@ namespace { hkl.reserve(cloud.size()); for (const Coord &s : cloud) { const float fh = a * s, fk = b * s, fl = c * s; // Coord operator* = dot = Miller index - const float h = std::round(fh), k = std::round(fk), l = std::round(fl); + const float h = std::rint(fh), k = std::rint(fk), l = std::rint(fl); const float dh = fh - h, dk = fk - k, dl = fl - l; if (dh * dh + dk * dk + dl * dl < tol_sq) hkl.push_back({static_cast(h), static_cast(k), static_cast(l)}); @@ -2479,8 +2482,8 @@ ProcessResult Rugnux::RunPipeline(RugnuxObserver *observer, bool write_output, b // multiple (2x/3x...) indexes every frame its true cell does, so both schemes reach the // same frame total and the count alone cannot tell them apart; the default then keeps // whichever ran first. When the two schemes tie on frames but their cell volumes are - // related by an integer factor >=2, the larger cell is that spurious supercell and the - // smaller is the true reduced cell: take it, regardless of scheme order. A near-integer + // related by an integer factor >=2, which of the pair is real is decided on the SPOTS by + // the arbiter below - regardless of scheme order, and in either direction. A near-integer // ratio separates a real axis multiplication from a centering coincidence. Volumes are // primitive (see above), so a pure setting difference is a ratio of 1 and never fires. bool integer_subcell = false; @@ -2499,7 +2502,6 @@ ProcessResult Rugnux::RunPipeline(RugnuxObserver *observer, bool write_output, b // which a real sub-lattice was ever seen is 3. const bool integer_multiple = nearest >= 2.0 && nearest <= 4.0 && std::abs(ratio - nearest) < 0.15; - integer_subcell = tied && !clearly_more && integer_multiple && vol < bp.vol; // Ask the data which of the pair is right instead of assuming the larger is // spurious: which cell accounts for more of the validation frames' spots (see @@ -3037,7 +3039,12 @@ ProcessResult Rugnux::RunPipeline(RugnuxObserver *observer, bool write_output, b // statistic that genuinely decides an axis multiple is the occupancy of the reflection class // the multiple adds - AbsenceEvidence in SearchSpaceGroup - and that needs integrated // intensities, so it cannot run here. - if (!cancelled_ && best.result.has_value() && ShortAxisPassEnabled()) { + // Not when a cell was given. RaiseFFTBoundForKnownCell has already lowered the floor for + // the standard pass against that cell, so this pass has nothing left to add - and it takes + // its settings from experiment_, which never carried the RAISED max bound, so on a cell + // longer than the default search it could only ever return a harmonic of what was asked + // for, tie on frames the way a sub-multiple always does, and be adopted over it. + if (!cancelled_ && !experiment_.GetUnitCell().has_value() && ShortAxisPassEnabled()) { auto short_settings = experiment_.GetIndexingSettings(); short_settings.FFT_MinUnitCell_A(IndexingSettings::fft_min_unit_cell_limit_A); short_settings.IndexingThreads(2);