From 3d382ce0a09c24e0021e0c8288d8a9d08eb03e6b Mon Sep 17 00:00:00 2001 From: Filip Leonarski Date: Mon, 26 Oct 2026 15:21:45 +0100 Subject: [PATCH] Refinement: an exception in a trial slot or in RefineModel no longer ends the run A slot builder thread that throws (gemmi's topology or hydrogen placement, bad_alloc) would call std::terminate - rugnux or the broker gone. The slot is now left out as one the card cannot take is, with a warning, and MakeOwnSlot's GPU engine catches std::exception, not only JFJochException. Both callers of RefineModel (after the merge, and --mode refine) catch std::exception: the refinement is reported NOT_PERFORMED with "refinement failed: " and the run writes everything else (cell e.s.d.s, unmerged MTZ, report), as the refinement is processing-inert. Test: --mode refine on a cell too large for the bulk-solvent grid comes back with REFINE= NOT_PERFORMED and the reason, not an exception. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015CeEGgpKdfkCTJb3YNMJcu (cherry picked from commit dc4e99fe4b3da2a9a3fa4290b5f70bfcc8c7bdb8) --- .../structure_refinement/Refinement.cpp | 9 +- rugnux/RugnuxRefine.cpp | 7 +- rugnux/RugnuxScaleMerge.cpp | 11 +- tests/RefineOnlyTest.cpp | 130 ++++++++++-------- 4 files changed, 98 insertions(+), 59 deletions(-) diff --git a/image_analysis/structure_refinement/Refinement.cpp b/image_analysis/structure_refinement/Refinement.cpp index 0f9478024..0cd8605a0 100644 --- a/image_analysis/structure_refinement/Refinement.cpp +++ b/image_analysis/structure_refinement/Refinement.cpp @@ -998,7 +998,7 @@ std::unique_ptr MakeOwnSlot(const gemmi::Structure &before_topology, co if (!ctx.engine) return nullptr; ctx.scale_gpu = std::make_unique(device, ctx.refl.Size(), 1); - } catch (const JFJochException &) { + } catch (const std::exception &) { return nullptr; } ctx.device = device; @@ -1299,7 +1299,12 @@ RefinementResult RefineModel(const gemmi::Structure &placed, const std::vector= 0 ? devices[k] : get_gpu()); #endif - own[k] = MakeOwnSlot(before_topology, ctx, devices[k], slot_threads, settings); + // a slot that cannot be made is left out, as where the card cannot take its engine + try { + own[k] = MakeOwnSlot(before_topology, ctx, devices[k], slot_threads, settings); + } catch (const std::exception &e) { + logger.Warning("Refinement: a trial slot could not be made ({}), the trials share fewer", e.what()); + } }); RefinePlacement(ctx, settings.group_b, result); diff --git a/rugnux/RugnuxRefine.cpp b/rugnux/RugnuxRefine.cpp index e70c7611c..8d897eab6 100644 --- a/rugnux/RugnuxRefine.cpp +++ b/rugnux/RugnuxRefine.cpp @@ -49,6 +49,11 @@ RefineOnlyResult RunRefineOnly(RugnuxReflections data, const gemmi::Structure &m d_min_limit > 0 ? fmt::format(", to {:.2f} A", d_min_limit) : std::string()); if (const gemmi::SpaceGroup *msg = model.find_spacegroup(); msg != nullptr && msg->number != sg->number) logger.Warning("The model names space group {}, the data {}: refined in the data's", msg->xhm(), sg->xhm()); - out.refinement = RefineModel(model, refl, data.cell, *sg, out.wavelength_A, logger, settings); + try { + out.refinement = RefineModel(model, refl, data.cell, *sg, out.wavelength_A, logger, settings); + } catch (const std::exception &e) { + out.refinement = RefinementResult(); + out.refinement.failure_reason = std::string("refinement failed: ") + e.what(); + } return out; } diff --git a/rugnux/RugnuxScaleMerge.cpp b/rugnux/RugnuxScaleMerge.cpp index 4d9eee3c0..113e46c6d 100644 --- a/rugnux/RugnuxScaleMerge.cpp +++ b/rugnux/RugnuxScaleMerge.cpp @@ -4811,8 +4811,15 @@ bool Rugnux::ScaleMergeAndSymmetry(PipelineLocals &p) { // A cut (the broker's deadline or cancel, Ctrl+C) cuts a running refinement short, keeping what it has settings.cancel = cancelled_.RunFlag(); const PlacedModelOutput &p = *placed_model_out; - RefinementResult refined = RefineModel(*p.model, *placed_merged, gemmi::UnitCell(p.cell), p.space_group, - experiment_.GetWavelength_A(), logger, settings); + // processing-inert: a refinement that fails is reported, and the run goes on to its other outputs + RefinementResult refined; + try { + refined = RefineModel(*p.model, *placed_merged, gemmi::UnitCell(p.cell), p.space_group, + experiment_.GetWavelength_A(), logger, settings); + } catch (const std::exception &e) { + refined = RefinementResult(); + refined.failure_reason = std::string("refinement failed: ") + e.what(); + } if (refined.ok) { result.outputs->refined_model = PlacedModelOutput{refined.model, p.cell, p.space_group}; result.outputs->refined_maps = refined.maps; diff --git a/tests/RefineOnlyTest.cpp b/tests/RefineOnlyTest.cpp index 670971f4a..c98c6e0ba 100644 --- a/tests/RefineOnlyTest.cpp +++ b/tests/RefineOnlyTest.cpp @@ -45,6 +45,62 @@ gemmi::Mtz MakeMtz(const std::vector> &columns, bool InTestSet(int h, int k, int l) { return (h * 7 + k * 13 + l * 17) % 10 == 0; } +// Waters scattered over the test cell, and their own |F|^2 as the data: the model as given scores well. +void SyntheticRefineInput(gemmi::Structure &st, RugnuxReflections &data) { + const gemmi::SpaceGroup *sg = gemmi::find_spacegroup_by_name("P 21 21 21"); + st.cell = test_cell; + st.spacegroup_hm = sg->xhm(); + st.setup_cell_images(); + gemmi::Model model(1); + gemmi::Chain chain("A"); + std::mt19937 rng(7); + std::uniform_real_distribution frac(0.0, 1.0); + for (int i = 0; i < 120; i++) { + gemmi::Residue res; + res.name = "HOH"; + res.seqid = gemmi::SeqId(i + 1, ' '); + res.het_flag = 'H'; + gemmi::Atom a; + a.name = "O"; + a.element = gemmi::El::O; + a.pos = test_cell.orthogonalize(gemmi::Fractional(frac(rng), frac(rng), frac(rng))); + a.b_iso = 15.0f; + a.occ = 1.0f; + res.atoms.push_back(a); + chain.residues.push_back(res); + } + model.chains.push_back(chain); + st.models.push_back(model); + + gemmi::DensityCalculator, float> dc; + dc.d_min = 2.0; + dc.grid.unit_cell = test_cell; + dc.grid.spacegroup = sg; + dc.set_refmac_compatible_blur(st.models[0]); + dc.put_model_density_on_grid(st.models[0]); + const gemmi::AsuData> f = + gemmi::transform_map_to_f_phi(dc.grid, true).prepare_asu_data(2.0, dc.blur); + + data.name = "synthetic.mtz"; + data.source = "MTZ"; + data.cell = test_cell; + data.space_group = sg->xhm(); + data.amplitudes = false; + for (const auto &hv : f.v) { + if (hv.hkl == gemmi::Miller{{0, 0, 0}}) + continue; + const double fc = std::abs(hv.value); + MergedReflection m; + m.h = hv.hkl[0]; + m.k = hv.hkl[1]; + m.l = hv.hkl[2]; + m.d = static_cast(test_cell.calculate_d(hv.hkl)); + m.I = static_cast(fc * fc); + m.sigma = static_cast(0.02 * fc * fc + 1.0); + data.reflections.push_back(m); + } +} + } // namespace TEST_CASE("RugnuxReflectionsFromMtz takes observed amplitudes over map coefficients", "[refine_only]") { @@ -124,62 +180,9 @@ TEST_CASE("ParseRugnuxReflections reads an MTZ from its bytes", "[refine_only]") TEST_CASE("RunRefineOnly scores a model against intensities, with amplitudes and free flags made for it", "[refine_only]") { - // Atoms scattered over the cell; the data are their own |F|^2, so the model as given scores well. - const gemmi::SpaceGroup *sg = gemmi::find_spacegroup_by_name("P 21 21 21"); gemmi::Structure st; - st.cell = test_cell; - st.spacegroup_hm = sg->xhm(); - st.setup_cell_images(); - gemmi::Model model(1); - gemmi::Chain chain("A"); - std::mt19937 rng(7); - std::uniform_real_distribution frac(0.0, 1.0); - for (int i = 0; i < 120; i++) { - gemmi::Residue res; - res.name = "HOH"; - res.seqid = gemmi::SeqId(i + 1, ' '); - res.het_flag = 'H'; - gemmi::Atom a; - a.name = "O"; - a.element = gemmi::El::O; - a.pos = test_cell.orthogonalize(gemmi::Fractional(frac(rng), frac(rng), frac(rng))); - a.b_iso = 15.0f; - a.occ = 1.0f; - res.atoms.push_back(a); - chain.residues.push_back(res); - } - model.chains.push_back(chain); - st.models.push_back(model); - - gemmi::DensityCalculator, float> dc; - dc.d_min = 2.0; - dc.grid.unit_cell = test_cell; - dc.grid.spacegroup = sg; - dc.set_refmac_compatible_blur(st.models[0]); - dc.put_model_density_on_grid(st.models[0]); - const gemmi::AsuData> f = - gemmi::transform_map_to_f_phi(dc.grid, true).prepare_asu_data(2.0, dc.blur); - RugnuxReflections data; - data.name = "synthetic.mtz"; - data.source = "MTZ"; - data.cell = test_cell; - data.space_group = sg->xhm(); - data.amplitudes = false; - for (const auto &hv : f.v) { - if (hv.hkl == gemmi::Miller{{0, 0, 0}}) - continue; - const double fc = std::abs(hv.value); - MergedReflection m; - m.h = hv.hkl[0]; - m.k = hv.hkl[1]; - m.l = hv.hkl[2]; - m.d = static_cast(test_cell.calculate_d(hv.hkl)); - m.I = static_cast(fc * fc); - m.sigma = static_cast(0.02 * fc * fc + 1.0); - data.reflections.push_back(m); - } - + SyntheticRefineInput(st, data); RefinementSettings settings; settings.score_only = true; settings.gpu = false; @@ -205,3 +208,22 @@ TEST_CASE("RunRefineOnly scores a model against intensities, with amplitudes and CHECK(text.find("REFINE_FREE_FLAGS= GENERATED") != std::string::npos); CHECK(text.find("REFINE_FRENCH_WILSON= TRUE") != std::string::npos); } + +TEST_CASE("RunRefineOnly reports a refinement that throws as not performed, with the reason", "[refine_only]") { + // The data's cell, a hundred metres on a side, makes the bulk-solvent mask's grid too large to allocate. + gemmi::Structure st; + RugnuxReflections data; + SyntheticRefineInput(st, data); + data.cell = gemmi::UnitCell(1e6, 1e6, 1e6, 90, 90, 90); + RefinementSettings settings; + settings.gpu = false; + settings.nthreads = 2; + Logger logger("RefineOnlyTest"); + RefineOnlyResult r; + REQUIRE_NOTHROW(r = RunRefineOnly(data, st, 0.0, settings, logger)); + CHECK_FALSE(r.refinement.ok); + CHECK(r.refinement.failure_reason.find("refinement failed") == 0); + const std::string text = RenderReportText(BuildRefineReportDocument("out", "synthetic.mtz", "model.cif", r), true); + CHECK(text.find("REFINE= NOT_PERFORMED") != std::string::npos); + CHECK(text.find("REFINE_REASON= refinement failed") != std::string::npos); +}