From de4de17abb98d4487f63f5555c2fabc4c329854d Mon Sep 17 00:00:00 2001 From: Filip Leonarski Date: Wed, 28 Oct 2026 15:11:06 +0100 Subject: [PATCH 1/3] RESULT-CHANGING: rugnux model stage: waits for the geometry walk's answer, stops on a model of another lattice, holds its memory in one host ledger Ordering. A canonical pass with a model to validate now waits, before deciding whether it is superseded, for the question GeometryWalkTakesRound asks: its own post-refinement fit and, for a move of less than a step, the speculative probe (AwaitGeometryWalkAnswer). On a card too small for the probe beside the merge the probe waited for the merge to end, so the answer could never be in before the model stage, and validation/MR/refinement ran on a merge the walk then replaced (measured on a large-cell set: a model validation on a wrong P1 supercell merge, OOM-killed above the logged host budget). The merge first re-declares what it still holds of the card (GPUMerge::Holds), and the probe is waited for only where it can then start (gpu_work_fits_beside_merges); otherwise nothing waits, as before. The cost - the probe's remaining time - falls only on --model runs whose fit moved the detector; fast_live does not speculate, so the broker's live mode is unchanged. Final outputs do not change: a superseded pass's files were rewritten by the walk's pass anyway. Model-cell check (owner approved). Where no change of basis of CellMappingOperators carries the model's cell onto the data's (CellsCorrespond tolerance), the unplaced validation stops with cell_mismatch and a reason naming both cells, instead of refractionalising the model into another lattice's cell. The report says MODEL_VALIDATION=CELL_MISMATCH and warns MODEL_CELL_MISMATCH (in place of MODEL_NOT_VALIDATED). Molecular replacement is still tried afterwards, as for any model not placed as given; placed paths (MR, ExpandIntoSubgroup, RefinableIn) are untouched. --model stays processing-inert. ModelValidation_WrongModelInAGroupOffTheDataMetric moves to cells within the tolerance so that it keeps testing what it tested. One host ledger. HostWorkerPool now knows the host budget and the bytes the run holds itself (HostWorkerPool::Held): workers may hold the pool's size or what the budget leaves beside the held bytes, whichever is less. ScaleMergeAndSymmetry holds its reflections, then engine + merge (+ post-refinement), and at the model stage the reflections and the engine where the ledger merge keeps it; a validation holds its own CPU grids; the change-of-basis frames reserve their grids per frame instead of all starting at once. The reserved == 0 escape stays, so a worker larger than what is left still runs, alone. Only what runs at the same time changes. Tests: ModelValidation_StopsOnAModelOfAnotherLattice, HostWorkerPool_HeldBytesShareTheBudget. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01HVeDQqN9qRD57vnShzTC37 --- common/CUDAWrapper.cpp | 5 ++ common/CUDAWrapper.h | 6 +++ common/HostMemory.cpp | 34 ++++++++++++- common/HostMemory.h | 38 +++++++++++--- docs/RUGNUX_REPORT.md | 4 +- .../structure_refinement/ModelValidation.cpp | 33 +++++++++++- .../structure_refinement/ModelValidation.h | 6 +++ rugnux/ReportDocument.h | 1 + rugnux/ResultReport.cpp | 5 +- rugnux/Rugnux.cpp | 7 ++- rugnux/Rugnux.h | 7 +++ rugnux/RugnuxPasses.cpp | 30 +++++++++++ rugnux/RugnuxScaleMerge.cpp | 22 +++++++- tests/HostMemoryTest.cpp | 42 +++++++++++++++ tests/ModelValidationTest.cpp | 51 +++++++++++++++++-- tests/RugnuxLargeTest.cpp | 1 + 16 files changed, 270 insertions(+), 22 deletions(-) diff --git a/common/CUDAWrapper.cpp b/common/CUDAWrapper.cpp index 3c311adcd..171dc380d 100644 --- a/common/CUDAWrapper.cpp +++ b/common/CUDAWrapper.cpp @@ -83,6 +83,11 @@ GPUWorkBeside::~GPUWorkBeside() { remove(gpu_work_beside_n); } +bool gpu_work_fits_beside_merges(size_t bytes) { + std::lock_guard lock(gpu_work_m); + return gpu_merges_n == 0 || fits_beside_merges(bytes); +} + bool wait_for_gpu_work_beside(std::chrono::seconds timeout) { std::unique_lock lock(gpu_work_m); return gpu_work_changed.wait_for(lock, timeout, [] { return gpu_work_beside_n == 0; }); diff --git a/common/CUDAWrapper.h b/common/CUDAWrapper.h index 537864081..80aa9d967 100644 --- a/common/CUDAWrapper.h +++ b/common/CUDAWrapper.h @@ -165,6 +165,12 @@ public: void Holds(size_t bytes); }; +// Whether work holding `bytes` of the card would start beside the main line's merges now (GPUWorkBeside with +// after_merges): no merge is held, or what they said they hold (GPUMerge::Holds) and `bytes` fit within 80% of +// the first card's total. For a main-line caller that is about to wait for such work, and must not where that +// work waits for its merge. Build-independent - it only counts. +bool gpu_work_fits_beside_merges(size_t bytes); + // Whether the card is too small for a probe pass beside the main line's image pass (12 GB, decided on the // first card's total memory), so that they take turns (GPUWorkBeside). Build-independent. bool gpu_takes_turns(); diff --git a/common/HostMemory.cpp b/common/HostMemory.cpp index 54c4a8b66..4c6771082 100644 --- a/common/HostMemory.cpp +++ b/common/HostMemory.cpp @@ -3,6 +3,8 @@ #include "HostMemory.h" +#include + #if defined(_WIN32) #ifndef NOMINMAX #define NOMINMAX @@ -47,6 +49,7 @@ HostWorkerPool &HostWorkerPool::Instance() { } HostWorkerPool::HostWorkerPool() { + SetBudget(0); SetSize(0); } @@ -57,14 +60,24 @@ void HostWorkerPool::SetSize(size_t bytes) { cv.notify_all(); } +void HostWorkerPool::SetBudget(size_t bytes) { + std::lock_guard lock(m); + budget = bytes > 0 ? bytes : DefaultHostMemoryBudget(); + cv.notify_all(); +} + +size_t HostWorkerPool::available() const { + return std::min(size, budget > held ? budget - held : 0); +} + size_t HostWorkerPool::Size() { std::lock_guard lock(m); - return size; + return available(); } HostWorkerPool::Reservation::Reservation(HostWorkerPool &pool, size_t bytes) : pool(pool), bytes(bytes) { std::unique_lock lock(pool.m); - pool.cv.wait(lock, [&] { return pool.reserved == 0 || pool.reserved + bytes <= pool.size; }); + pool.cv.wait(lock, [&] { return pool.reserved == 0 || pool.reserved + bytes <= pool.available(); }); pool.reserved += bytes; } @@ -75,3 +88,20 @@ HostWorkerPool::Reservation::~Reservation() { } pool.cv.notify_all(); } + +HostWorkerPool::Held::Held(HostWorkerPool &pool, size_t bytes) : pool(pool), bytes(0) { + Set(bytes); +} + +HostWorkerPool::Held::~Held() { + Set(0); +} + +void HostWorkerPool::Held::Set(size_t new_bytes) { + { + std::lock_guard lock(pool.m); + pool.held = pool.held - bytes + new_bytes; + bytes = new_bytes; + } + pool.cv.notify_all(); +} diff --git a/common/HostMemory.h b/common/HostMemory.h index 07dae951b..312d1f999 100644 --- a/common/HostMemory.h +++ b/common/HostMemory.h @@ -24,20 +24,26 @@ size_t DefaultHostMemoryBudget(); // its reflections, its merge, the detector-sized arrays. constexpr double HOST_WORKER_POOL_FRACTION = 0.5; -// One pool for those buffers, process-wide. A worker reserves its bytes before it makes them and hands -// them back when it is done; one that would take the pool past its size waits until others have handed -// theirs back - except that a reservation made while nothing is reserved is always let through, however -// large. So how many such workers run at once is decided by their bytes, not by the thread count, and -// however the steps holding them nest or run side by side; what each worker computes, and where its -// result goes, does not change. A worker must not reserve again while it holds a reservation, and none -// may reserve on ParallelFor's pool: a holder's own parallel passes need those threads. Work spread over -// that pool is bounded by its count instead (Size() / bytes each). +// One pool for those buffers, process-wide, held in one ledger with what the run holds beside them. The run +// says what it holds - its reflections, its merge engine, a validation's own grids - as Held bytes, and the +// workers may hold at once the pool's size or what the budget leaves beside the Held bytes, whichever is +// less: so the two together never claim more than the budget, however large the run's own part is. A worker +// reserves its bytes before it makes them and hands them back when it is done; one that would take the pool +// past that waits until others have handed theirs back - except that a reservation made while no worker holds +// one is always let through, however large. So how many such workers run at once is decided by their bytes, +// not by the thread count, and however the steps holding them nest or run side by side; what each worker +// computes, and where its result goes, does not change. A worker must not reserve again while it holds a +// reservation, and none may reserve on ParallelFor's pool: a holder's own parallel passes need those threads. +// Work spread over that pool is bounded by its count instead (Size() / bytes each). class HostWorkerPool { public: static HostWorkerPool &Instance(); // The pool's size in bytes; 0 sets it back to HOST_WORKER_POOL_FRACTION of DefaultHostMemoryBudget(). void SetSize(size_t bytes); + // The budget the pool and the Held bytes share; 0 sets it back to DefaultHostMemoryBudget(). + void SetBudget(size_t bytes); + // What the workers may hold at once now: the pool's size, or the budget less the Held bytes if less. [[nodiscard]] size_t Size(); class Reservation { @@ -51,10 +57,26 @@ public: size_t bytes; }; + // Bytes the run holds beside the workers for as long as this lives; never waits. + class Held { + public: + Held(HostWorkerPool &pool, size_t bytes); + ~Held(); + Held(const Held &) = delete; + Held &operator=(const Held &) = delete; + void Set(size_t bytes); // what it holds from now on + private: + HostWorkerPool &pool; + size_t bytes; + }; + private: HostWorkerPool(); + size_t available() const; // Size(), with the lock held std::mutex m; std::condition_variable cv; size_t size = 0; + size_t budget = 0; + size_t held = 0; size_t reserved = 0; }; diff --git a/docs/RUGNUX_REPORT.md b/docs/RUGNUX_REPORT.md index e3b9ceb6f..97a5959a9 100644 --- a/docs/RUGNUX_REPORT.md +++ b/docs/RUGNUX_REPORT.md @@ -250,7 +250,7 @@ and a run that produced a textbook data set read identically for their first thr consumer switches on a code rather than parsing a sentence: `NO_LATTICE`, `INDEXING_AMBIGUITY`, `SYMMETRY_AMBIGUITY`, `CENTERING_UNTESTED`, `UNUSABLE_MERGE`, `LOW_COMPLETENESS`, `SWEEP_GAPS`, `GONIO_SCALE`, `SPINDLE_CAP`, `ANISOTROPY`, `TWINNING`, `PSEUDO_TRANSLATION`, - `LATTICE_TRANSLATION`, `MODEL_HAND`, `MODEL_NOT_VALIDATED`, `MODEL_FREE_SET`, `MODEL_SPACE_GROUP`, `REFERENCE_MISMATCH`, `CANCELLED`, `RESOLUTION_FIT`, + `LATTICE_TRANSLATION`, `MODEL_HAND`, `MODEL_NOT_VALIDATED`, `MODEL_CELL_MISMATCH`, `MODEL_FREE_SET`, `MODEL_SPACE_GROUP`, `REFERENCE_MISMATCH`, `CANCELLED`, `RESOLUTION_FIT`, `FLIGHT_PATH`, `GEOMETRY_NOT_CONVERGED`, `SCALING_NOT_CONVERGED`, `HARMONIC_CONTAMINATION`, `SUPERCELL_POSSIBLE`, `MULTIPLE_LATTICES`, `ICE_RINGS`, `POWDER_RINGS`, `BEAM_CENTRE_OFF`, `OVERLOADS`, `OVERLAPS`. `NONE` when nothing fired. A code appears if and only if its warning fired, so the flags and the `WARNING:` lines are two renderings of one list. Thresholds are @@ -973,7 +973,7 @@ tried (a prediction, `--mr`) or its null could not be built. | key | meaning | |---|---| -| `MODEL_VALIDATION` | `PERFORMED` \| `NOT_PERFORMED` (with `MODEL_VALIDATION_REASON`, and no R-factors) | +| `MODEL_VALIDATION` | `PERFORMED` \| `NOT_PERFORMED` \| `CELL_MISMATCH` (both with `MODEL_VALIDATION_REASON`, and no R-factors). `CELL_MISMATCH`: no change of basis (an axis permutation, a sign flip, I- against C-centred) carries the model's cell onto the data's within 5% in each length and 3 deg in each angle, so the model is of another lattice - a cell of another volume, an axis of another length - and is not put into the data's cell to be scored; the reason names both cells, and the `MODEL_CELL_MISMATCH` warning says so. Molecular replacement is still tried, as for any model that is not placed as given | | `MODEL_PLACEMENT` | `RIGID_BODY` (the model fits against its null in the answer's merge or the twin hypothesis' one) \| `MR` (placed by molecular replacement) \| `NOT_PLACED` | | `MODEL_FIT` | `ACCEPTED` \| `REJECTED` — the rigid body's verdict: whether the model may decide anything. Absent where no rigid body was tried | | `MODEL_FIT_STATISTIC` | What the verdict was taken on; `R_WORK` | diff --git a/image_analysis/structure_refinement/ModelValidation.cpp b/image_analysis/structure_refinement/ModelValidation.cpp index c2b2ca749..a50f5e853 100644 --- a/image_analysis/structure_refinement/ModelValidation.cpp +++ b/image_analysis/structure_refinement/ModelValidation.cpp @@ -653,11 +653,24 @@ ModelValidationResult Validate(const std::vector &merged, // each dataset in whichever indexing its own run happened to pick. // A model handed over already placed in the data's cell (molecular replacement) is on the data's axes. if (data_cell.is_crystal() && placed == nullptr) { + const std::vector mapping = CellMappingOperators(st.cell, data_cell); + if (mapping.empty()) { + const auto cell_text = [](const gemmi::UnitCell &c) { + return fmt::format("{:.2f} {:.2f} {:.2f} {:.1f} {:.1f} {:.1f}", c.a, c.b, c.c, c.alpha, c.beta, c.gamma); + }; + result.cell_mismatch = true; + result.failure_reason = fmt::format( + "the model's cell {} ({}) is not this lattice's {} (volume ratio {:.2f}): no change of basis " + "carries one onto the other", cell_text(st.cell), sg->xhm(), cell_text(data_cell), + st.cell.volume / data_cell.volume); + logger.Warning("Model validation: {}", result.failure_reason); + return result; + } const gemmi::GroupOps model_gops = sg->operations(); const bool same_point_group = data_space_group != nullptr && same_rotations(data_space_group->operations(), model_gops); std::vector frames{gemmi::Op::identity()}; - for (const gemmi::Op &op : CellMappingOperators(st.cell, data_cell)) { + for (const gemmi::Op &op : mapping) { bool seen = false; for (const gemmi::Op &kept : frames) seen = seen || same_frame(kept, op, model_gops); @@ -687,11 +700,17 @@ ModelValidationResult Validate(const std::vector &merged, std::vector *> labellings{&obs}; for (const auto &v : relabelled) labellings.push_back(&v); + // Each holds its grids against the host's worker pool (HostWorkerPool) while it scores: as many + // run at once as fit beside what the run holds, and each scores the same either way. auto score_frame = [&](size_t i) { gemmi::Structure trial = st; const gemmi::SpaceGroup *trial_sg = sg; if (i > 0 && !change_model_basis(trial, trial_sg, frames[i])) return; // no origin shift names the transformed group; not a basis we can take + const HostWorkerPool::Reservation reserved( + HostWorkerPool::Instance(), + MODEL_FACTORS_BYTES_PER_GRID_POINT + * ModelGridPoints(data_cell, trial_sg, std::max(d_min, FRAME_PROBE_D_MIN))); refractionalize_into(trial, data_cell); trial.setup_cell_images(); // Each frame at the data's best indexing: the alternative indexings are only probed @@ -878,6 +897,12 @@ ModelValidationResult Validate(const std::vector &merged, #else (void) rigid_body_gpu; #endif + // The real model's grids, where its structure factors are made on the CPU, are held for as long as the + // validation runs, beside the run's own bytes (HostWorkerPool::Held): the null's replicates below, and any + // other worker, take only what the two leave of the host budget. + const HostWorkerPool::Held model_grids( + HostWorkerPool::Instance(), + sf_gpu == nullptr ? MODEL_FACTORS_BYTES_PER_GRID_POINT * ModelGridPoints(ucell, sg, d_min) : 0); // --- Fcalc (atomic) via electron density on a grid + FFT, plus a flat bulk-solvent mask -> Fmask. // A lambda because the rigid-body step below moves the model and then needs both again, and it @@ -1255,6 +1280,12 @@ ModelValidationResult Validate(const std::vector &merged, if ((sf_gpu != nullptr && null_d_min == sf_gpu->DMin()) || (sf_null != nullptr && null_d_min == sf_null->DMin())) replicate_bytes = 0; #endif + if (replicate_bytes > 0 && decision_pending) { + const size_t room = HostWorkerPool::Instance().Size(); + logger.Info("Model validation: the null's replicates hold {} each on the host, {} of the worker pool beside " + "what the run holds - {} at a time", GBytes(replicate_bytes), GBytes(room), + std::clamp(room / replicate_bytes, 1, NULL_REPLICATES)); + } auto run_replicate = [&](int i) { const HostWorkerPool::Reservation reserved(HostWorkerPool::Instance(), replicate_bytes); std::mt19937 rng(NULL_SEED + 1 + i); diff --git a/image_analysis/structure_refinement/ModelValidation.h b/image_analysis/structure_refinement/ModelValidation.h index a42be37d3..b0a5b1e8f 100644 --- a/image_analysis/structure_refinement/ModelValidation.h +++ b/image_analysis/structure_refinement/ModelValidation.h @@ -35,6 +35,12 @@ struct ModelValidationResult { // usable space group. A caller that asked for a model must be able to say so rather than finish // quietly with no R-free and no maps, which reads exactly like a run that was never asked. std::string failure_reason; + // Why it did not run, where the reason is that the model's cell is not the data's lattice: no change of + // basis of CellMappingOperators carries one onto the other within CellsCorrespond's tolerance - a model put + // into a cell four times its volume, an axis a third longer than the data's. An R-factor of a model + // refractionalised into another lattice's cell measures nothing, so the validation stops there, and + // failure_reason names both cells. A placed model (molecular replacement) is already in the data's cell. + bool cell_mismatch = false; double r_work = 0.0; double r_free = 0.0; double k_sol = 0.0; diff --git a/rugnux/ReportDocument.h b/rugnux/ReportDocument.h index bfd81ae35..dc21b20b3 100644 --- a/rugnux/ReportDocument.h +++ b/rugnux/ReportDocument.h @@ -35,6 +35,7 @@ namespace PathologyCode { constexpr const char *TWINNING = "TWINNING"; // twinning indicated constexpr const char *MODEL_HAND = "MODEL_HAND"; // data and model in opposite hands constexpr const char *MODEL_NOT_VALIDATED = "MODEL_NOT_VALIDATED"; // a model was given, not usable + constexpr const char *MODEL_CELL_MISMATCH = "MODEL_CELL_MISMATCH"; // the model's cell is not the data's lattice constexpr const char *MODEL_FREE_SET = "MODEL_FREE_SET"; // the model as given was scored on a free set drawn here constexpr const char *MODEL_SPACE_GROUP = "MODEL_SPACE_GROUP"; // the model's group replaced the one fixed with -S constexpr const char *REFERENCE_MISMATCH = "REFERENCE_MISMATCH"; // the reference MTZ does not match; free flags not inherited diff --git a/rugnux/ResultReport.cpp b/rugnux/ResultReport.cpp index 1a85a59ba..7a1609206 100644 --- a/rugnux/ResultReport.cpp +++ b/rugnux/ResultReport.cpp @@ -2656,13 +2656,14 @@ ReportDocument BuildReportDocument(const std::string &output_prefix, if (!result.model_placement.empty()) Add(s, KeyEnum("MODEL_PLACEMENT", result.model_placement, {"RIGID_BODY", "MR", "NOT_PLACED"})); if (!mv.ok) { - Add(s, KeyEnum("MODEL_VALIDATION", "NOT_PERFORMED", {"PERFORMED", "NOT_PERFORMED"})); + Add(s, KeyEnum("MODEL_VALIDATION", mv.cell_mismatch ? "CELL_MISMATCH" : "NOT_PERFORMED", + {"PERFORMED", "NOT_PERFORMED", "CELL_MISMATCH"})); Add(s, KeyText("MODEL_VALIDATION_REASON", mv.failure_reason)); Add(s, Blank()); Add(s, Prose(" A model was given but could not be used or placed, so there are no R-factors and no\n" " maps. Everything else in this report is unaffected: the merge does not depend on the model.")); } else { - Add(s, KeyEnum("MODEL_VALIDATION", "PERFORMED", {"PERFORMED", "NOT_PERFORMED"})); + Add(s, KeyEnum("MODEL_VALIDATION", "PERFORMED", {"PERFORMED", "NOT_PERFORMED", "CELL_MISMATCH"})); Add(s, KeyInt("MODEL_SPACE_GROUP_NUMBER", mv.model_space_group_number)); Add(s, KeyReal("R_WORK", mv.r_work, "{:.4f}")); Add(s, KeyReal("R_FREE", mv.r_free, "{:.4f}")); diff --git a/rugnux/Rugnux.cpp b/rugnux/Rugnux.cpp index 33de9fb4c..74d63beda 100644 --- a/rugnux/Rugnux.cpp +++ b/rugnux/Rugnux.cpp @@ -331,9 +331,11 @@ Rugnux::Rugnux(JFJochReader &reader, DiffractionExperiment experiment, // What the run plans its host memory against, decided once from the machine's total (HostMemory.h): // work that would not fit it side by side runs one part after the other, and the large per-worker - // buffers of the model's steps are held a share of it at a time. Only when things run changes. + // buffers of the model's steps are held a share of it at a time - of what the run's own reflections, + // merge and grids leave of it (HostWorkerPool::Held). Only when things run changes. const size_t machine = get_host_total_memory(); host_budget_ = config_.host_memory_budget_bytes > 0 ? config_.host_memory_budget_bytes : DefaultHostMemoryBudget(); + HostWorkerPool::Instance().SetBudget(host_budget_); HostWorkerPool::Instance().SetSize( std::max(1, static_cast(HOST_WORKER_POOL_FRACTION * static_cast(host_budget_)))); Logger("Rugnux").Info("Host memory budget {:.1f} GB ({}): what does not fit it side by side runs in series", @@ -647,12 +649,13 @@ void Rugnux::StartSpeculativeGeometryProbe(std::shared_ptr run, const st auto sp = std::make_shared(); sp->run = run; sp->hand = hand; + sp->gpu_bytes = run->IndexingProbeGPUBytes(); // A probe that fails leaves no memo, and the walk's own probe then runs it again and meets the // failure itself. It runs beside the merge of the pass that started it, on the same card: a merge // allocation that does not fit beside it waits for it to end (GPUWorkBeside) - and where the probe's // bytes and the merge's do not fit the card together, the probe waits for that merge to end instead // (GPUMerge). Its two passes run one after the other, so it holds at most what one does. - const size_t probe_bytes = run->IndexingProbeGPUBytes(); + const size_t probe_bytes = sp->gpu_bytes; Logger("Rugnux").Debug("Speculative geometry probe: {:.2f} GB of the card", probe_bytes / 1e9); sp->done = std::async(std::launch::async, [run, hand, probe_bytes] { GPUWorkBeside beside(/*after_merges=*/true, probe_bytes); diff --git a/rugnux/Rugnux.h b/rugnux/Rugnux.h index f240ce0f3..d8e41c6c9 100644 --- a/rugnux/Rugnux.h +++ b/rugnux/Rugnux.h @@ -57,6 +57,7 @@ class JFJochReader; class JFJochHDF5Reader; struct JFJochReaderRawImage; struct PipelineLocals; // RugnuxPipeline.h, private to the implementation +class GPUMerge; // CUDAWrapper.h class IndexerThreadPool; // Offline reprocessing of a stored Jungfraujoch HDF5 dataset, shared by rugnux (all of its --mode @@ -1350,6 +1351,7 @@ class Rugnux { std::shared_ptr run; std::shared_ptr hand; // the walk's probe at the geometry in hand, after run's std::future done; + size_t gpu_bytes = 0; // what it holds of the card at its most (IndexingProbeGPUBytes) }; std::shared_ptr speculative_probe_; bool speculate_geometry_probe_ = false; @@ -1373,6 +1375,11 @@ class Rugnux { // Whether the geometry walk after the canonical pass will take a round, asked by that pass from // what it already has in hand; `fit` is its post-refinement running beside the merge (RefinedPasses). [[nodiscard]] bool GeometryWalkTakesRound(const ProcessResult &result, const std::future &fit) const; + // Wait until GeometryWalkTakesRound has its answer, for a pass about to spend its model stage on a merge + // the walk may replace. `merge_on_card` is what the pass's merge still holds of the card; the probe is let + // beside it (GPUMerge::Holds), and not waited for where it would not fit - it waits for that merge. + void AwaitGeometryWalkAnswer(const ProcessResult &result, const std::future &fit, GPUMerge *gpu_merge, + size_t merge_on_card) const; // The part of FirstPassInputKey that is the experiment and the spot-finding settings; without the // goniometer's angles where rotation_angles is false (SpotFindingKey). [[nodiscard]] std::vector ExperimentKey(bool rotation_angles = true) const { diff --git a/rugnux/RugnuxPasses.cpp b/rugnux/RugnuxPasses.cpp index 2dc4d1d36..ba9367a8b 100644 --- a/rugnux/RugnuxPasses.cpp +++ b/rugnux/RugnuxPasses.cpp @@ -875,6 +875,36 @@ bool Rugnux::GeometryWalkTakesRound(const ProcessResult &result, const std::futu return at_hand && at_fit && ValidationEvidencePrefers(*at_hand, *at_fit); } +// For a canonical pass with a model to validate: the model stage - validation, molecular replacement and +// refinement, the largest host and card footprint of a run - is spent on this pass's merge only where the +// walk is not going to replace it, so the pass waits here for the question GeometryWalkTakesRound asks. +// The answer needs the pass's own post-refinement and, for a move of less than a step, the speculative +// probe; that probe waits for the merge where the two do not fit the card together (GPUMerge), so the +// merge first says what it still holds, and the probe is waited for only where it can then start. A pass +// whose fit does not move the detector started no probe and waits for nothing past its fit; one where the +// walk takes a round is superseded, and the model stage runs once, on the merge the walk keeps. +void Rugnux::AwaitGeometryWalkAnswer(const ProcessResult &result, const std::future &fit, GPUMerge *gpu_merge, + size_t merge_on_card) const { + if (!speculate_geometry_probe_ || cancelled_ || !prepass_detector_geometry_) + return; + if (fit.valid()) + fit.wait(); + if (!result.post_refine || !result.post_refine->detector_refined || result.post_refine->large_move + || config_.verify_first_pass_memo || !speculative_probe_ + || speculative_probe_->done.wait_for(std::chrono::seconds(0)) == std::future_status::ready) + return; + if (gpu_merge) + gpu_merge->Holds(merge_on_card); + if (!gpu_work_fits_beside_merges(speculative_probe_->gpu_bytes)) { + Logger("Rugnux").Debug("Geometry walk: the probe does not fit the card beside the merge's {:.2f} GB - " + "the model stage does not wait for it", static_cast(merge_on_card) / 1e9); + return; + } + Logger logger("Rugnux"); + TimingMark mark(logger, "geometry walk answer wait (model)"); + speculative_probe_->done.wait(); +} + // The refined passes of the rotation two-pass, from the second pass on: everything after the // rotation-scale walk (see RunAllPasses). Empty where they ran beside the walk and it overturned them. std::optional Rugnux::RefinedPasses(RugnuxObserver *observer, ProcessResult pass1, diff --git a/rugnux/RugnuxScaleMerge.cpp b/rugnux/RugnuxScaleMerge.cpp index 0d7d38991..b01f3364d 100644 --- a/rugnux/RugnuxScaleMerge.cpp +++ b/rugnux/RugnuxScaleMerge.cpp @@ -33,6 +33,7 @@ #include "../reader/JFJochHDF5Reader.h" #include "../common/JFJochMath.h" #include "../common/ParallelFor.h" +#include "../common/HostMemory.h" #include "../common/Logger.h" #include "../common/AzimuthalIntegrationMapping.h" #include "../common/AzimuthalIntegrationProfile.h" @@ -1224,6 +1225,9 @@ bool Rugnux::ScaleMergeAndSymmetry(PipelineLocals &p) { } const size_t reflection_bytes = n_reflections * sizeof(Reflection); const auto host_gb = [](size_t bytes) { return static_cast(bytes) / 1e9; }; + // ... and what the large per-worker buffers of the model's steps, and any beside the merge, are held + // against: the same budget, less this (HostWorkerPool::Held), so the two together never claim more. + HostWorkerPool::Held host_held(HostWorkerPool::Instance(), reflection_bytes); std::future guard_ahead; if (merge_first_part && !(postrefine_probe_ && !geometry_prepass && postrefine_probe_only_)) { // The guard's merge runs beside the main engine's ingest. Where an engine of the guard's frames and @@ -1526,6 +1530,7 @@ bool Rugnux::ScaleMergeAndSymmetry(PipelineLocals &p) { constexpr size_t POSTREFINE_RANKED_BYTES = 16; // PostRefineRotationGeometry's ranking, per partial const size_t host_postrefine = n_reflections * (sizeof(PostRefinePartial) + POSTREFINE_RANKED_BYTES); bool postrefine_beside = p.postrefine_ahead.valid(); + host_held.Set(host_engine + host_merge + (postrefine_beside ? host_postrefine : 0)); if (postrefine_beside && host_engine + host_merge + host_postrefine > host_budget_) { logger.Info("Host memory: the post-refinement ({:.1f} GB) does not fit beside the merge ({:.1f} GB, " "{:.1f} GB held) within the {:.1f} GB budget - it is done before the merge", @@ -3909,6 +3914,10 @@ bool Rugnux::ScaleMergeAndSymmetry(PipelineLocals &p) { // So is a canonical pass the geometry walk is going to replace, where that answer is in by now // (GeometryWalkTakesRound). A refined pass run beside the rotation-scale walk is superseded too // where the walk overturns it (see RunAllPasses); it waits here for the walk's answer. + // With a model to validate, the walk's answer is waited for too: the model stage is the largest part of + // such a run, and spent on a merge the walk replaces it is spent twice (AwaitGeometryWalkAnswer). + if (config_.model && write_output && !geometry_prepass && result.consensus_cell) + AwaitGeometryWalkAnswer(result, p.postrefine_ahead, p.gpu_merge, rsm && rsm->OnGPU() ? rsm->DeviceBytes() : 0); superseded = (!cancelled_ && quality_guard_pass1_ && quality_guard_pass1_->has_merge_statistics && ((bragg_adaptive_ && bkg_starved_fraction_ @@ -4096,6 +4105,10 @@ bool Rugnux::ScaleMergeAndSymmetry(PipelineLocals &p) { // that one started on the forecast decides as the one it stands in for would. if (model_run && !ledger_ahead.valid()) rsm.reset(); + // What the run holds beside the model's steps from here on: its reflections, and the merge engine where the + // ledger's merge still runs on it. + if (model_run) + host_held.Set(reflection_bytes + (rsm ? rsm->HostIngestBytes() + rsm->HostMergeBytes() : 0)); const size_t merge_on_card = rsm && rsm->OnGPU() ? rsm->DeviceBytes() : 0; ModelValidationSchedule first_schedule; first_schedule.gpu_resident_bytes = merge_on_card; @@ -5341,8 +5354,13 @@ bool Rugnux::ScaleMergeAndSymmetry(PipelineLocals &p) { std::string why = result.model_validation ? result.model_validation->failure_reason : std::string(); if (why.empty() && result.model_prior && !result.model_prior->ok) why = "the model could not be searched for: " + result.model_prior->failure_reason; - if (!(result.model_validation && result.model_validation->ok) && !why.empty()) - result.warnings.push_back({PathologyCode::MODEL_NOT_VALIDATED, "Model validation did not run: " + why}); + if (!(result.model_validation && result.model_validation->ok) && !why.empty()) { + if (result.model_validation && result.model_validation->cell_mismatch) + result.warnings.push_back({PathologyCode::MODEL_CELL_MISMATCH, "The model does not describe the " + "data's lattice, so it was not validated: " + why}); + else + result.warnings.push_back({PathologyCode::MODEL_NOT_VALIDATED, "Model validation did not run: " + why}); + } // The outcome, as notes of the step that decided it if (result.model_prior) { const MolecularReplacementResult &mp = *result.model_prior; diff --git a/tests/HostMemoryTest.cpp b/tests/HostMemoryTest.cpp index e58d17ebd..d566e4dc5 100644 --- a/tests/HostMemoryTest.cpp +++ b/tests/HostMemoryTest.cpp @@ -46,3 +46,45 @@ TEST_CASE("HostWorkerPool_BoundsWorkersByBytes", "[HostMemory]") { pool.SetSize(0); CHECK(pool.Size() == static_cast(HOST_WORKER_POOL_FRACTION * static_cast(DefaultHostMemoryBudget()))); } + +// One ledger: what the run holds (Held) and what the workers reserve together stay within the budget. Workers +// take what the Held bytes leave of it, up to the pool's size, and a worker larger than that runs alone rather +// than never. +TEST_CASE("HostWorkerPool_HeldBytesShareTheBudget", "[HostMemory]") { + HostWorkerPool &pool = HostWorkerPool::Instance(); + pool.SetBudget(100); + pool.SetSize(50); + CHECK(pool.Size() == 50); + const auto most_at_once = [&](size_t bytes) { + std::atomic running{0}, most{0}, done{0}; + std::vector workers; + for (int i = 0; i < 8; i++) + workers.emplace_back([&] { + const HostWorkerPool::Reservation reserved(pool, bytes); + const int now = ++running; + int seen = most.load(); + while (now > seen && !most.compare_exchange_weak(seen, now)) {} + std::this_thread::sleep_for(std::chrono::milliseconds(20)); + --running; + ++done; + }); + for (auto &w : workers) + w.join(); + CHECK(done == 8); + return most.load(); + }; + { + HostWorkerPool::Held run(pool, 70); // leaves 30 of the budget: the pool's 50 does not fit beside it + CHECK(pool.Size() == 30); + CHECK(most_at_once(10) <= 3); + run.Set(20); // leaves 80: the pool's own size bounds again + CHECK(pool.Size() == 50); + CHECK(most_at_once(10) <= 5); + run.Set(100); // nothing left: one at a time, each still runs + CHECK(pool.Size() == 0); + CHECK(most_at_once(10) == 1); + } + CHECK(pool.Size() == 50); + pool.SetBudget(0); + pool.SetSize(0); +} diff --git a/tests/ModelValidationTest.cpp b/tests/ModelValidationTest.cpp index 7b622a91d..5aedff6f8 100644 --- a/tests/ModelValidationTest.cpp +++ b/tests/ModelValidationTest.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -952,9 +953,12 @@ TEST_CASE("ModelValidation_WarmUpChangesNothing", "[ModelValidation]") { // reserved for the zone as it is counted, so the validation runs instead of failing on the device. TEST_CASE("ModelValidation_WrongModelInAGroupOffTheDataMetric", "[ModelValidation]") { Logger logger("ModelValidation_WrongModelInAGroupOffTheDataMetric"); - const auto data_path = WriteTemp("off_metric_data.pdb", ClusterPdb().c_str()); + // A cubic model on a cell within the tolerance of the data's orthorhombic one, so that it is scored + // (a model of another lattice is not: ModelValidation_StopsOnAModelOfAnotherLattice). + const auto data_path = WriteTemp("off_metric_data.pdb", + ClusterPdb("CRYST1 36.000 37.000 38.000 90.00 90.00 90.00 P 21 21 21 4\n").c_str()); const auto wrong_path = WriteTemp("off_metric_wrong.pdb", - ClusterPdb("CRYST1 40.000 40.000 40.000 90.00 90.00 90.00 I 21 3 24\n", 7).c_str()); + ClusterPdb("CRYST1 37.000 37.000 37.000 90.00 90.00 90.00 I 21 3 24\n", 7).c_str()); auto obs = ModelReferenceIntensities(ReadModel(data_path), {}, {}, 2.0, logger); REQUIRE(obs.size() > 1000); for (size_t i = 0; i < obs.size(); i++) { @@ -963,7 +967,7 @@ TEST_CASE("ModelValidation_WrongModelInAGroupOffTheDataMetric", "[ModelValidatio obs[i].rfree_flag = (i % 20) == 0; } const auto result = - ValidateAgainstModel(obs, UnitCell{.a = 30, .b = 34, .c = 38, .alpha = 90, .beta = 90, .gamma = 90}, + ValidateAgainstModel(obs, UnitCell{.a = 36, .b = 37, .c = 38, .alpha = 90, .beta = 90, .gamma = 90}, ReadModel(wrong_path), logger, gemmi::find_spacegroup_by_name("P 21 21 21")); CHECK(result.ok); CHECK(!result.adopted_model_enantiomorph); @@ -972,6 +976,47 @@ TEST_CASE("ModelValidation_WrongModelInAGroupOffTheDataMetric", "[ModelValidatio std::filesystem::remove(wrong_path); } +// A model whose cell no change of basis carries onto the data's is of another lattice: it is not put into the +// data's cell to be scored, and the reason names both cells. A synthetic cluster; the model's cells are the +// data's with the volume four times as large, and with one axis a third longer. +TEST_CASE("ModelValidation_StopsOnAModelOfAnotherLattice", "[ModelValidation]") { + Logger logger("ModelValidation_StopsOnAModelOfAnotherLattice"); + const auto data_path = WriteTemp("other_lattice_data.pdb", ClusterPdb().c_str()); + auto obs = ModelReferenceIntensities(ReadModel(data_path), {}, {}, 2.5, logger); + REQUIRE(obs.size() > 500); + for (size_t i = 0; i < obs.size(); i++) { + obs[i].F = std::sqrt(std::max(0.0f, obs[i].I)); + obs[i].rfree_flag = (i % 20) == 0; + } + const UnitCell data_cell{.a = 30, .b = 34, .c = 38, .alpha = 90, .beta = 90, .gamma = 90}; + const gemmi::SpaceGroup *data_sg = gemmi::find_spacegroup_by_name("P 21 21 21"); + + const std::pair models[] = { + {"CRYST1 60.000 68.000 38.000 90.00 90.00 90.00 P 21 21 21 4\n", "60.00 68.00 38.00"}, + {"CRYST1 30.000 34.000 52.000 90.00 90.00 90.00 P 21 21 21 4\n", "30.00 34.00 52.00"}}; + for (const auto &[cryst, model_cell] : models) { + CAPTURE(cryst); + const auto model_path = WriteTemp("other_lattice_model.pdb", ClusterPdb(cryst).c_str()); + const auto result = ValidateAgainstModel(obs, data_cell, ReadModel(model_path), logger, data_sg, + /*probe_indexing_ambiguity=*/true, 1, 1.0); + CHECK_FALSE(result.ok); + CHECK(result.cell_mismatch); + CHECK_THAT(result.failure_reason, Catch::Matchers::ContainsSubstring("30.00 34.00 38.00")); + CHECK_THAT(result.failure_reason, Catch::Matchers::ContainsSubstring(model_cell)); + std::filesystem::remove(model_path); + } + + // The same lattice on other axes is a description of it, and is scored. + const auto same_path = WriteTemp("other_lattice_same.pdb", + ClusterPdb("CRYST1 31.000 33.500 38.500 90.00 90.00 90.00 P 21 21 21 4\n").c_str()); + const auto same = ValidateAgainstModel(obs, data_cell, ReadModel(same_path), logger, data_sg, false, 1, 1.0); + CHECK(same.ok); + CHECK_FALSE(same.cell_mismatch); + + std::filesystem::remove(data_path); + std::filesystem::remove(same_path); +} + // The groups a model may relabel the data into: those the merged intensities cannot separate from the // data's - an enantiomorph, or a group whose screws the centring already hides - and no other. TEST_CASE("ModelValidation_SameIntensities", "[ModelValidation]") { diff --git a/tests/RugnuxLargeTest.cpp b/tests/RugnuxLargeTest.cpp index bbdd8cca3..95dea17ff 100644 --- a/tests/RugnuxLargeTest.cpp +++ b/tests/RugnuxLargeTest.cpp @@ -186,4 +186,5 @@ TEST_CASE("Rugnux_HostBudgetChangesOnlyTheSchedule", "[large]") { CHECK(slurp((dir / "tight.hkl").string()) == hkl); std::filesystem::remove_all(dir); HostWorkerPool::Instance().SetSize(0); // process-wide: back to the machine's for the tests after this one + HostWorkerPool::Instance().SetBudget(0); } From 26810d42d7703ddd61f0c63db248d15c3edeba22 Mon Sep 17 00:00:00 2001 From: Filip Leonarski Date: Wed, 28 Oct 2026 15:38:50 +0100 Subject: [PATCH 2/3] rugnux: the model stage asks the geometry walk where the card is let go, not at the analyses On a 16 GB card the merge engine alone (10.3 GB on a large cell) leaves no room for the speculative probe (4.8 GB), so asking at the superseded decision before the analyses could never let the probe run and the model stage still went ahead on the merge the walk replaced. The question now waits until just before the model stage, where that stage lets go of the card anyway - the indexers, and the merge engine where the finalist ledger does not keep it - and does so first where the answer still waits on the probe (GeometryWalkProbePending, AwaitGeometryWalkProbe). A pass the walk will replace is then marked superseded there: no model stage, no report, no files. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01HVeDQqN9qRD57vnShzTC37 --- rugnux/Rugnux.h | 11 ++++++----- rugnux/RugnuxPasses.cpp | 35 ++++++++++++++++++----------------- rugnux/RugnuxScaleMerge.cpp | 23 +++++++++++++++++++---- 3 files changed, 43 insertions(+), 26 deletions(-) diff --git a/rugnux/Rugnux.h b/rugnux/Rugnux.h index d8e41c6c9..f68b97f31 100644 --- a/rugnux/Rugnux.h +++ b/rugnux/Rugnux.h @@ -1375,11 +1375,12 @@ class Rugnux { // Whether the geometry walk after the canonical pass will take a round, asked by that pass from // what it already has in hand; `fit` is its post-refinement running beside the merge (RefinedPasses). [[nodiscard]] bool GeometryWalkTakesRound(const ProcessResult &result, const std::future &fit) const; - // Wait until GeometryWalkTakesRound has its answer, for a pass about to spend its model stage on a merge - // the walk may replace. `merge_on_card` is what the pass's merge still holds of the card; the probe is let - // beside it (GPUMerge::Holds), and not waited for where it would not fit - it waits for that merge. - void AwaitGeometryWalkAnswer(const ProcessResult &result, const std::future &fit, GPUMerge *gpu_merge, - size_t merge_on_card) const; + // For a pass about to spend its model stage on a merge the walk may replace: waits for `fit`, and says whether + // GeometryWalkTakesRound's answer still waits on the speculative probe. + [[nodiscard]] bool GeometryWalkProbePending(const ProcessResult &result, const std::future &fit) const; + // Wait for that probe, where it can start beside what the merge still holds of the card (`merge_on_card`, + // said to GPUMerge); return at once where it cannot - it waits for that merge. + void AwaitGeometryWalkProbe(GPUMerge *gpu_merge, size_t merge_on_card) const; // The part of FirstPassInputKey that is the experiment and the spot-finding settings; without the // goniometer's angles where rotation_angles is false (SpotFindingKey). [[nodiscard]] std::vector ExperimentKey(bool rotation_angles = true) const { diff --git a/rugnux/RugnuxPasses.cpp b/rugnux/RugnuxPasses.cpp index ba9367a8b..d260bffab 100644 --- a/rugnux/RugnuxPasses.cpp +++ b/rugnux/RugnuxPasses.cpp @@ -877,31 +877,32 @@ bool Rugnux::GeometryWalkTakesRound(const ProcessResult &result, const std::futu // For a canonical pass with a model to validate: the model stage - validation, molecular replacement and // refinement, the largest host and card footprint of a run - is spent on this pass's merge only where the -// walk is not going to replace it, so the pass waits here for the question GeometryWalkTakesRound asks. -// The answer needs the pass's own post-refinement and, for a move of less than a step, the speculative -// probe; that probe waits for the merge where the two do not fit the card together (GPUMerge), so the -// merge first says what it still holds, and the probe is waited for only where it can then start. A pass -// whose fit does not move the detector started no probe and waits for nothing past its fit; one where the -// walk takes a round is superseded, and the model stage runs once, on the merge the walk keeps. -void Rugnux::AwaitGeometryWalkAnswer(const ProcessResult &result, const std::future &fit, GPUMerge *gpu_merge, - size_t merge_on_card) const { +// walk is not going to replace it, so the pass asks GeometryWalkTakesRound once its answer is in. That needs +// the pass's own post-refinement, waited for here, and, for a move of less than a step, the speculative +// probe: true where the answer still waits on that probe (AwaitGeometryWalkProbe). A pass whose fit does not +// move the detector started no probe and waits for nothing past its fit. +bool Rugnux::GeometryWalkProbePending(const ProcessResult &result, const std::future &fit) const { if (!speculate_geometry_probe_ || cancelled_ || !prepass_detector_geometry_) - return; + return false; if (fit.valid()) fit.wait(); - if (!result.post_refine || !result.post_refine->detector_refined || result.post_refine->large_move - || config_.verify_first_pass_memo || !speculative_probe_ - || speculative_probe_->done.wait_for(std::chrono::seconds(0)) == std::future_status::ready) - return; + return result.post_refine && result.post_refine->detector_refined && !result.post_refine->large_move + && !config_.verify_first_pass_memo && speculative_probe_ + && speculative_probe_->done.wait_for(std::chrono::seconds(0)) != std::future_status::ready; +} + +// The probe waits for the merge where the two do not fit the card together (GPUMerge), so the merge first +// says what it still holds, and the probe is waited for only where it can then start. +void Rugnux::AwaitGeometryWalkProbe(GPUMerge *gpu_merge, size_t merge_on_card) const { if (gpu_merge) gpu_merge->Holds(merge_on_card); + Logger logger("Rugnux"); if (!gpu_work_fits_beside_merges(speculative_probe_->gpu_bytes)) { - Logger("Rugnux").Debug("Geometry walk: the probe does not fit the card beside the merge's {:.2f} GB - " - "the model stage does not wait for it", static_cast(merge_on_card) / 1e9); + logger.Debug("Geometry walk: the probe does not fit the card beside the merge's {:.2f} GB - the model " + "stage does not wait for it", static_cast(merge_on_card) / 1e9); return; } - Logger logger("Rugnux"); - TimingMark mark(logger, "geometry walk answer wait (model)"); + TimingMark mark(logger, "geometry walk probe wait (model)"); speculative_probe_->done.wait(); } diff --git a/rugnux/RugnuxScaleMerge.cpp b/rugnux/RugnuxScaleMerge.cpp index b01f3364d..79700f225 100644 --- a/rugnux/RugnuxScaleMerge.cpp +++ b/rugnux/RugnuxScaleMerge.cpp @@ -3914,10 +3914,6 @@ bool Rugnux::ScaleMergeAndSymmetry(PipelineLocals &p) { // So is a canonical pass the geometry walk is going to replace, where that answer is in by now // (GeometryWalkTakesRound). A refined pass run beside the rotation-scale walk is superseded too // where the walk overturns it (see RunAllPasses); it waits here for the walk's answer. - // With a model to validate, the walk's answer is waited for too: the model stage is the largest part of - // such a run, and spent on a merge the walk replaces it is spent twice (AwaitGeometryWalkAnswer). - if (config_.model && write_output && !geometry_prepass && result.consensus_cell) - AwaitGeometryWalkAnswer(result, p.postrefine_ahead, p.gpu_merge, rsm && rsm->OnGPU() ? rsm->DeviceBytes() : 0); superseded = (!cancelled_ && quality_guard_pass1_ && quality_guard_pass1_->has_merge_statistics && ((bragg_adaptive_ && bkg_starved_fraction_ @@ -4049,6 +4045,25 @@ bool Rugnux::ScaleMergeAndSymmetry(PipelineLocals &p) { Note("cut: the model is left out"); return true; }; + // With a model to validate, the walk's answer is waited for: the model stage is the largest part of such a + // run, and spent on a merge the walk replaces it is spent twice (GeometryWalkProbePending). Where the answer + // waits on the probe, what the model stage would let go of the card first goes now - the indexers, and the + // merge engine where the ledger's merge does not keep it - so that the probe can run. + if (result.consensus_cell && write_output && config_.model && !geometry_prepass && !superseded + && GeometryWalkProbePending(result, p.postrefine_ahead)) { + if (census_future.valid()) + census_future.wait(); + p.indexer_pool.reset(); + if (!ledger_ahead.valid()) + rsm.reset(); + AwaitGeometryWalkProbe(p.gpu_merge, rsm && rsm->OnGPU() ? rsm->DeviceBytes() : 0); + if (GeometryWalkTakesRound(result, p.postrefine_ahead)) { + superseded = true; + logger.Info("This pass is going to be re-run, so it validates no model, makes no report and writes " + "no files."); + Note("this pass is to be re-run (see the log); it writes nothing"); + } + } const bool model_run = result.consensus_cell && write_output && config_.model && !geometry_prepass && !superseded && !model_cut(); // The merge's own shell bounds, so CC(model, data) is reported on the shells CC1/2 was. From 037412472cdc1985be22876e93d485322e6f68e6 Mon Sep 17 00:00:00 2001 From: Filip Leonarski Date: Wed, 28 Oct 2026 17:11:18 +0100 Subject: [PATCH 3/3] ModelValidation: a centred description of the same primitive cell is not a cell mismatch The unimodular set of CellMappingOperators does not reach a centred cell from its primitive one, so the cell check took a P21 model of a C-centred orthorhombic lattice (its twin subgroup's primitive cell, same primitive volume) for another lattice, and the run lost the twin-hypothesis placement it had made before (open battery set, MODEL_CHOICE P21). A mismatch now also needs the primitive volumes to differ beyond what CellsCorrespond's 5% in three lengths allows; the four-fold supercell and the third-longer axis are still caught. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01HVeDQqN9qRD57vnShzTC37 --- docs/RUGNUX_REPORT.md | 2 +- .../structure_refinement/ModelValidation.cpp | 18 ++++++++++++++---- .../structure_refinement/ModelValidation.h | 9 +++++---- tests/ModelValidationTest.cpp | 8 ++++++++ 4 files changed, 28 insertions(+), 9 deletions(-) diff --git a/docs/RUGNUX_REPORT.md b/docs/RUGNUX_REPORT.md index 97a5959a9..cc3dca85f 100644 --- a/docs/RUGNUX_REPORT.md +++ b/docs/RUGNUX_REPORT.md @@ -973,7 +973,7 @@ tried (a prediction, `--mr`) or its null could not be built. | key | meaning | |---|---| -| `MODEL_VALIDATION` | `PERFORMED` \| `NOT_PERFORMED` \| `CELL_MISMATCH` (both with `MODEL_VALIDATION_REASON`, and no R-factors). `CELL_MISMATCH`: no change of basis (an axis permutation, a sign flip, I- against C-centred) carries the model's cell onto the data's within 5% in each length and 3 deg in each angle, so the model is of another lattice - a cell of another volume, an axis of another length - and is not put into the data's cell to be scored; the reason names both cells, and the `MODEL_CELL_MISMATCH` warning says so. Molecular replacement is still tried, as for any model that is not placed as given | +| `MODEL_VALIDATION` | `PERFORMED` \| `NOT_PERFORMED` \| `CELL_MISMATCH` (both with `MODEL_VALIDATION_REASON`, and no R-factors). `CELL_MISMATCH`: no change of basis (an axis permutation, a sign flip, I- against C-centred) carries the model's cell onto the data's within 5% in each length and 3 deg in each angle, and the two primitive cells differ in volume by more than that allows, so the model is of another lattice - a cell of another volume, an axis of another length - and is not put into the data's cell to be scored (a centred description of the same primitive cell is not caught here); the reason names both cells, and the `MODEL_CELL_MISMATCH` warning says so. Molecular replacement is still tried, as for any model that is not placed as given | | `MODEL_PLACEMENT` | `RIGID_BODY` (the model fits against its null in the answer's merge or the twin hypothesis' one) \| `MR` (placed by molecular replacement) \| `NOT_PLACED` | | `MODEL_FIT` | `ACCEPTED` \| `REJECTED` — the rigid body's verdict: whether the model may decide anything. Absent where no rigid body was tried | | `MODEL_FIT_STATISTIC` | What the verdict was taken on; `R_WORK` | diff --git a/image_analysis/structure_refinement/ModelValidation.cpp b/image_analysis/structure_refinement/ModelValidation.cpp index a50f5e853..f97bddaad 100644 --- a/image_analysis/structure_refinement/ModelValidation.cpp +++ b/image_analysis/structure_refinement/ModelValidation.cpp @@ -653,16 +653,26 @@ ModelValidationResult Validate(const std::vector &merged, // each dataset in whichever indexing its own run happened to pick. // A model handed over already placed in the data's cell (molecular replacement) is on the data's axes. if (data_cell.is_crystal() && placed == nullptr) { + // Another lattice, where no change of basis of CellMappingOperators carries the model's cell onto the + // data's AND the two primitive cells differ in volume by more than CellsCorrespond lets three lengths + // differ. The second condition keeps a description of the same lattice that the unimodular set does not + // reach - a centred cell against its primitive one (a C-centred orthorhombic cell and the primitive + // monoclinic one of its twin-subgroup), the hexagonal and rhombohedral settings - on the path it took. const std::vector mapping = CellMappingOperators(st.cell, data_cell); - if (mapping.empty()) { + const auto primitive_volume = [](const gemmi::UnitCell &c, const gemmi::SpaceGroup *g) { + return c.volume / static_cast(g != nullptr ? g->operations().cen_ops.size() : 1); + }; + const double volume_ratio = primitive_volume(st.cell, sg) / primitive_volume(data_cell, data_space_group); + constexpr double SAME_VOLUME = 1.05 * 1.05 * 1.05; // CellsCorrespond's 5% in each length + if (mapping.empty() && (volume_ratio > SAME_VOLUME || volume_ratio < 1.0 / SAME_VOLUME)) { const auto cell_text = [](const gemmi::UnitCell &c) { return fmt::format("{:.2f} {:.2f} {:.2f} {:.1f} {:.1f} {:.1f}", c.a, c.b, c.c, c.alpha, c.beta, c.gamma); }; result.cell_mismatch = true; result.failure_reason = fmt::format( - "the model's cell {} ({}) is not this lattice's {} (volume ratio {:.2f}): no change of basis " - "carries one onto the other", cell_text(st.cell), sg->xhm(), cell_text(data_cell), - st.cell.volume / data_cell.volume); + "the model's cell {} ({}) is not this lattice's {} (primitive volume ratio {:.2f}): no change " + "of basis carries one onto the other", cell_text(st.cell), sg->xhm(), cell_text(data_cell), + volume_ratio); logger.Warning("Model validation: {}", result.failure_reason); return result; } diff --git a/image_analysis/structure_refinement/ModelValidation.h b/image_analysis/structure_refinement/ModelValidation.h index b0a5b1e8f..7725d7483 100644 --- a/image_analysis/structure_refinement/ModelValidation.h +++ b/image_analysis/structure_refinement/ModelValidation.h @@ -36,10 +36,11 @@ struct ModelValidationResult { // quietly with no R-free and no maps, which reads exactly like a run that was never asked. std::string failure_reason; // Why it did not run, where the reason is that the model's cell is not the data's lattice: no change of - // basis of CellMappingOperators carries one onto the other within CellsCorrespond's tolerance - a model put - // into a cell four times its volume, an axis a third longer than the data's. An R-factor of a model - // refractionalised into another lattice's cell measures nothing, so the validation stops there, and - // failure_reason names both cells. A placed model (molecular replacement) is already in the data's cell. + // basis of CellMappingOperators carries one onto the other within CellsCorrespond's tolerance, and the + // primitive cells differ in volume beyond it - a model put into a cell four times its volume, an axis a + // third longer than the data's. An R-factor of a model refractionalised into another lattice's cell + // measures nothing, so the validation stops there, and failure_reason names both cells. A placed model + // (molecular replacement) is already in the data's cell. bool cell_mismatch = false; double r_work = 0.0; double r_free = 0.0; diff --git a/tests/ModelValidationTest.cpp b/tests/ModelValidationTest.cpp index 5aedff6f8..9ae2f0473 100644 --- a/tests/ModelValidationTest.cpp +++ b/tests/ModelValidationTest.cpp @@ -1006,6 +1006,14 @@ TEST_CASE("ModelValidation_StopsOnAModelOfAnotherLattice", "[ModelValidation]") std::filesystem::remove(model_path); } + // A centred description of a cell of the same primitive volume is not taken for another lattice: it is + // left to the scoring, as before (where it reads as a model that does not fit). + const auto centred_path = WriteTemp("other_lattice_centred.pdb", + ClusterPdb("CRYST1 60.000 34.000 38.000 90.00 90.00 90.00 C 2 2 21 8\n").c_str()); + const auto centred = ValidateAgainstModel(obs, data_cell, ReadModel(centred_path), logger, data_sg, false, 1, 1.0); + CHECK_FALSE(centred.cell_mismatch); + std::filesystem::remove(centred_path); + // The same lattice on other axes is a description of it, and is scored. const auto same_path = WriteTemp("other_lattice_same.pdb", ClusterPdb("CRYST1 31.000 33.500 38.500 90.00 90.00 90.00 P 21 21 21 4\n").c_str());