From bf0f9590009dd20ef4e0e3a88c98c9ddd87b5dcf Mon Sep 17 00:00:00 2001 From: Filip Leonarski Date: Sat, 31 Oct 2026 12:11:03 +0100 Subject: [PATCH] rugnux report: a model that was not placed leaves every line outside MODEL VALIDATION as a run without one writes it ProcessResult::ModelPlaced() (MODEL_PLACEMENT RIGID_BODY or MR) is the one test of an accepted model. The MODEL_HAND warning and SYMMETRY_HYPOTHESIS_n TESTED_BY_MODEL now require it: an inverted anomalous map at a rejected model's atoms, or a hypothesis a rejected model was scored on, said something about the model in the data-side part of the report. The wrong-model null (nullmodel.py) now checks that principle directly: every report line outside the model section - KEY=, prose and WARNING: lines - must equal the no-model run's, provenance aside. ResultReport_RejectedModelLeavesTheDataLines tests it for a rejected model and a model of another lattice. Co-Authored-By: Claude Opus 5.5 --- docs/RUGNUX_MODEL_AND_REFERENCE.md | 2 +- docs/RUGNUX_REPORT.md | 2 +- rugnux/ResultReport.cpp | 5 ++- rugnux/Rugnux.h | 4 ++ tests/ResultReportTest.cpp | 68 ++++++++++++++++++++++++++++++ tools/battery/README.md | 7 +-- tools/battery/nullmodel.py | 46 +++++++++++++------- 7 files changed, 112 insertions(+), 22 deletions(-) diff --git a/docs/RUGNUX_MODEL_AND_REFERENCE.md b/docs/RUGNUX_MODEL_AND_REFERENCE.md index 8c3da7c74..cde751bfe 100644 --- a/docs/RUGNUX_MODEL_AND_REFERENCE.md +++ b/docs/RUGNUX_MODEL_AND_REFERENCE.md @@ -152,7 +152,7 @@ these letters. 5. **What an accepted model may do** (class b), applied to the reflections before they are written: - **hand** — where the data's group is the enantiomorph of the model's, the files take the model's group as their label. No reflection moves and I(+)/I(-) stay as measured. Vetoed where - the anomalous difference map read at the model's atoms is inverted (warning `MODEL_HAND`). Where the + the anomalous difference map read at the model's atoms is inverted (warning `MODEL_HAND`, raised only for a placed model). Where the group was fixed with `-S` (or by the reference), the model's hand replacing it is warned about (`MODEL_SPACE_GROUP`); - **indexing** — where the lattice admits alternative indexings and no `-z` already settled it, diff --git a/docs/RUGNUX_REPORT.md b/docs/RUGNUX_REPORT.md index 23eb510b4..9994522b6 100644 --- a/docs/RUGNUX_REPORT.md +++ b/docs/RUGNUX_REPORT.md @@ -1034,7 +1034,7 @@ untwinned or twinned by a law of the refused group outside it, and each *other* refused group perfectly twinned by a law of its coset - each of them merges into the refused group alike. The report lists them, on every run, as `SYMMETRY_HYPOTHESES=` (their number) and `SYMMETRY_HYPOTHESIS_= | | ADOPTED / ADOPTED_TWINNED / SIBLING_TWINNED | -TESTED_BY_MODEL / UNTESTED`; a sibling's group is named by its rotations alone (symmorphic), its screws +TESTED_BY_MODEL / UNTESTED` (`TESTED_BY_MODEL` only where the model was placed, `MODEL_PLACEMENT=` `RIGID_BODY` or `MR`: a rejected model leaves these lines as a run without one writes them); a sibling's group is named by its rotations alone (symmorphic), its screws left to the model. They cost nothing without a model. With `--model`, where the model's own group is a sibling's (it has the sibling's rotations on the data's axes), that sibling's merge is the twin hypothesis the model is scored on, exactly as a subgroup's above, and the model is refined in its own group there. diff --git a/rugnux/ResultReport.cpp b/rugnux/ResultReport.cpp index 1ca5cf040..132ece884 100644 --- a/rugnux/ResultReport.cpp +++ b/rugnux/ResultReport.cpp @@ -1295,7 +1295,8 @@ ReportDocument BuildReportDocument(const std::string &output_prefix, Add(s, KeyText(fmt::format("SYMMETRY_HYPOTHESIS_{}", i + 1).c_str(), fmt::format("{} | {} | {} | {}", h.space_group, h.twin_law.empty() ? "NONE" : h.twin_law, h.kind, - h.tested_by_model ? "TESTED_BY_MODEL" : "UNTESTED"))); + h.tested_by_model && result.ModelPlaced() ? "TESTED_BY_MODEL" + : "UNTESTED"))); } Add(s, Prose(fmt::format( " The refused {} leaves these symmetry hypotheses: the adopted group, untwinned and twinned\n" @@ -3032,7 +3033,7 @@ ReportDocument BuildReportDocument(const std::string &output_prefix, Add(s, Prose(" The model asserts the other enantiomorph of the written space group, and that\n" " assertion was NOT taken up: merged intensities cannot check it, and the anomalous\n" " difference map is the only measurement here sensitive to the hand.")); - if (mv.anomalous_hands_disagree) + if (mv.anomalous_hands_disagree && result.ModelPlaced()) Warn(doc, PathologyCode::MODEL_HAND, fmt::format( "The anomalous density at the model's atoms is inverted ({} reads {:.1f} sigma, deeper " "than the highest peak) - the data and the model are in opposite hands. Either the model " diff --git a/rugnux/Rugnux.h b/rugnux/Rugnux.h index 223ac20c9..96b6d4062 100644 --- a/rugnux/Rugnux.h +++ b/rugnux/Rugnux.h @@ -781,6 +781,10 @@ struct ProcessResult { // How the --model was placed: RIGID_BODY (it fits against its null in the answer's merge or the twin // hypothesis' one), MR (molecular replacement placed it) or NOT_PLACED. Empty without a model. std::string model_placement; + // The one test of whether the model was accepted: it was placed. A model that was not - of another + // lattice, not placed by molecular replacement, a rigid-body fit no better than its null - leaves every + // line of the report outside its own section as a run without it writes them (the wrong-model null). + bool ModelPlaced() const { return model_placement == "RIGID_BODY" || model_placement == "MR"; } // What molecular replacement found, where it ran; failure_reason says why where it could not. std::optional model_prior; // What refinement made of the placed model; unset where nothing was placed (or refine was off). diff --git a/tests/ResultReportTest.cpp b/tests/ResultReportTest.cpp index 31f113d14..d6ec42621 100644 --- a/tests/ResultReportTest.cpp +++ b/tests/ResultReportTest.cpp @@ -2,6 +2,7 @@ // SPDX-License-Identifier: GPL-3.0-only #include +#include #include "../common/DiffractionExperiment.h" #include "../image_analysis/scale_merge/Merge.h" @@ -585,6 +586,73 @@ TEST_CASE("ResultReport_ModelValidationSection", "[Diagnostics]") { // get_gpu_description collapses repeats, so a four-card machine reads as one line rather than the // same name four times. Build-independent: without CUDA there are no names and it is empty. +// A model that was not placed - of another lattice, or with a rigid-body fit no better than its null - leaves +// every line of the report outside its own section as the run without a model writes them (the wrong-model +// null, tools/battery/nullmodel.py). Only a placed model may add to them. +TEST_CASE("ResultReport_RejectedModelLeavesTheDataLines", "[Diagnostics]") { + DiffractionExperiment x(DetJF(1)); + x.ImagesPerTrigger(600); + + ProcessResult none; + none.images_processed = 600; + none.consensus_cell = UnitCell{.a = 79.0f, .b = 79.0f, .c = 38.0f, + .alpha = 90.0f, .beta = 90.0f, .gamma = 90.0f}; + none.space_group = *gemmi::find_spacegroup_by_number(96); + none.space_group_search = SearchSpaceGroupResult{}; + none.space_group_search->refused_point_group_hm = "422"; + none.symmetry_hypotheses = {{"P 43 21 2", "", "ADOPTED", false}}; + + const auto outside_model_section = [&](const ProcessResult &r) { + std::string text = RenderResultReport("p", "in.h5", x, r); + const size_t section = text.find("5. MODEL VALIDATION"); + if (section != std::string::npos) + text.erase(section, text.find("END OF REPORT") - section); + std::string out; + std::istringstream in(text); + for (std::string line; std::getline(in, line);) + if (!line.starts_with("DATE=") && !line.starts_with("WALL_TIME=")) + out += line + "\n"; + return out; + }; + const std::string expected = outside_model_section(none); + + ModelValidationResult rejected; + rejected.ok = true; + rejected.model_name = "model.cif"; + rejected.model_space_group_number = 96; + rejected.r_work = 0.58; + rejected.r_free = 0.59; + rejected.fit_tested = true; + rejected.model_fits = false; + rejected.anomalous_hands_disagree = true; + rejected.anomalous_deepest_site = "SD MET A1"; + rejected.anomalous_deepest_sigma = -5.5; + ProcessResult wrong = none; + wrong.model_validation = rejected; + wrong.model_placement = "NOT_PLACED"; + wrong.symmetry_hypotheses[0].tested_by_model = true; + CHECK(outside_model_section(wrong) == expected); + + ModelValidationResult mismatch; + mismatch.model_name = "model.cif"; + mismatch.cell_mismatch = true; + mismatch.failure_reason = "the model's cell is not this lattice's"; + ProcessResult other_lattice = none; + other_lattice.model_validation = mismatch; + other_lattice.model_placement = "NOT_PLACED"; + const auto mismatch_text = RenderResultReport("p", "in.h5", x, other_lattice); + CHECK(mismatch_text.find("\nMODEL_VALIDATION= CELL_MISMATCH\n") != std::string::npos); + CHECK(outside_model_section(other_lattice) == expected); + + // The same model placed: the hypothesis it was scored on and the inverted anomalous density are its findings. + ProcessResult placed = wrong; + placed.model_validation->model_fits = true; + placed.model_placement = "RIGID_BODY"; + const auto placed_text = RenderResultReport("p", "in.h5", x, placed); + CHECK(placed_text.find("| ADOPTED | TESTED_BY_MODEL") != std::string::npos); + CHECK(placed_text.find("MODEL_HAND") != std::string::npos); +} + TEST_CASE("ResultReport_GpuDescription", "[Diagnostics]") { const auto names = get_gpu_names(); CHECK(names.size() == static_cast(std::max(0, get_gpu_count()))); diff --git a/tools/battery/README.md b/tools/battery/README.md index 53c35d5c6..b98ccb05a 100644 --- a/tools/battery/README.md +++ b/tools/battery/README.md @@ -327,9 +327,10 @@ and scores every wrong-model run against the no-model run of the same set (`null - **(a) data**: `p.hkl` byte for byte; the `p.mtz` reflections value for value (space group, cell, columns, every number - the file differs by the command line in its history); `p.cif` without its command line. -- **(b) decisions**: every `KEY=` line of `p_report.txt` outside MODEL VALIDATION - space group, - `SOHNCKE_SPACE_GROUP` and `SPACE_GROUP_ENANTIOMORPH`, cell, resolution cut, twinning, warnings - with - only `DATE`, `COMMAND_LINE` and `WALL_TIME` left out. +- **(b) decisions**: every line of `p_report.txt` outside MODEL VALIDATION - the `KEY=` lines (space + group, `SOHNCKE_SPACE_GROUP` and `SPACE_GROUP_ENANTIOMORPH`, cell, resolution cut, twinning, warnings), + the prose and the `WARNING:` lines - with only `DATE`, `COMMAND_LINE` and `WALL_TIME` left out. A + rejected model leaves the rest of the report exactly as a run without it writes it. - **(c) a proper null**: a model of another lattice is recognised as such - `MODEL_VALIDATION` is `CELL_MISMATCH`, so it is not scored - and molecular replacement does not place it; otherwise `MODEL_FIT` is not `ACCEPTED` (`REJECTED`: the fit is no better than the same model in random orientations), molecular replacement - which runs wherever the rigid body does not diff --git a/tools/battery/nullmodel.py b/tools/battery/nullmodel.py index 6140acc48..9b92e220d 100644 --- a/tools/battery/nullmodel.py +++ b/tools/battery/nullmodel.py @@ -8,9 +8,10 @@ wrong-model run with the no-model run of the same set: (a) data p.hkl byte for byte; p.mtz reflection by reflection (group, cell, columns, every value - the file itself differs by the command line in its history); p.cif with its command line left out - (b) decisions every KEY= line of p_report.txt outside the MODEL VALIDATION section (space group, - SOHNCKE group and SPACE_GROUP_ENANTIOMORPH, cell, resolution cut, twinning, - warnings, ...), provenance lines aside + (b) decisions every line of p_report.txt outside the MODEL VALIDATION section - KEY= lines (space + group, SOHNCKE group and SPACE_GROUP_ENANTIOMORPH, cell, resolution cut, twinning, + warnings, ...), prose and WARNING: lines alike - provenance lines aside: a rejected + model leaves the rest of the report as a run without it writes it (c) null the model is recognised as wrong: MODEL_VALIDATION is CELL_MISMATCH (a model of another lattice, not scored) and molecular replacement does not place it, or else MODEL_FIT is not ACCEPTED, molecular replacement - which @@ -22,6 +23,7 @@ wrong-model run with the no-model run of the same set: (Wilson 1950); the scale, anisotropic B, bulk solvent and rigid body fitted to the working set can only pull it a few points below that, so R_FREE >= RANDOM_R_FREE_MIN. """ +import difflib import hashlib import json import os @@ -41,7 +43,7 @@ RANDOM_R_FREE_MIN = 0.50 # lines of p_report.txt that say how and when the run was made, not what it decided PROVENANCE = {"DATE", "COMMAND_LINE", "WALL_TIME"} MODEL_SECTION = "5. MODEL VALIDATION" -SECTION_TITLE = re.compile(r"^# \d+\. [A-Z]") +BANNER = "# " + "*" * 78 def f(x, fmt): @@ -60,19 +62,27 @@ def sha256(path): return h.hexdigest() -def decision_keys(path): - """KEY= lines of a report outside the model section, provenance left out.""" - out, in_model = {}, False +def data_lines(path): + """Every line of a report outside the model section - KEY= lines, prose, WARNING: lines - with the + provenance KEY= lines and the banners left out. A section's title is the line after a banner.""" + out, in_model, prev = [], False, "" for line in open(path, errors="replace"): - # a numbered section's title ("# 5. MODEL VALIDATION"), not a table row that starts with a number - if SECTION_TITLE.match(line): + line = line.rstrip("\n") + if prev == BANNER and line.startswith("# "): in_model = line[3:].strip() == MODEL_SECTION - m = score.REPORT_LINE.match(line.rstrip("\n")) - if m and not in_model and m.group(1) not in PROVENANCE: - out[m.group(1)] = m.group(2).strip() + prev = line + m = score.REPORT_LINE.match(line) + if in_model or line == BANNER or (m and m.group(1) in PROVENANCE): + continue + out.append(line) return out +def decision_keys(lines): + """The KEY= lines among data_lines, as a dict.""" + return {m.group(1): m.group(2).strip() for m in map(score.REPORT_LINE.match, lines) if m} + + def same_mtz(a, b): ma, mb = gemmi.read_mtz_file(a), gemmi.read_mtz_file(b) if (ma.spacegroup.number != mb.spacegroup.number or ma.cell.parameters != mb.cell.parameters @@ -95,12 +105,16 @@ def score_pair(none_wd, wd): ("p.cif", same_cif)): a, b = os.path.join(none_wd, name), os.path.join(wd, name) files[name] = os.path.exists(a) and os.path.exists(b) and same(a, b) - ka, kb = (decision_keys(os.path.join(d, "p_report.txt")) for d in (none_wd, wd)) + la, lb = (data_lines(os.path.join(d, "p_report.txt")) for d in (none_wd, wd)) + ka, kb = decision_keys(la), decision_keys(lb) moved = sorted(k for k in set(ka) | set(kb) if ka.get(k) != kb.get(k)) + # the principle itself: outside the model section the report is the no-model run's, line for line + lines_moved = [ln for ln in difflib.unified_diff(la, lb, lineterm="", n=0) + if ln[:1] in "+-" and not ln.startswith(("+++", "---"))] r_free = score.fnum(rep.get("R_FREE")) row = { "data_same": all(files.values()), "files_differ": [n for n, s in files.items() if not s], - "decisions_moved": moved, + "decisions_moved": moved, "lines_moved": lines_moved, "sg": kb.get("SPACE_GROUP_NAME"), "enantiomorph": kb.get("SPACE_GROUP_ENANTIOMORPH"), "model_validation": rep.get("MODEL_VALIDATION"), "model_fit": rep.get("MODEL_FIT"), "model_placement": rep.get("MODEL_PLACEMENT"), @@ -118,6 +132,8 @@ def score_pair(none_wd, wd): why.append("data differ: " + ", ".join(row["files_differ"])) if moved: why.append("decisions moved: " + ", ".join(moved[:6]) + (" ..." if len(moved) > 6 else "")) + elif lines_moved: + why.append(f"{len(lines_moved)} report line(s) moved outside MODEL VALIDATION: {lines_moved[0][:120]}") if row["model_validation"] == "CELL_MISMATCH": # a model of another lattice, recognised as such: not scored, and molecular replacement must not place it if row["model_placement"] == "MR": @@ -146,7 +162,7 @@ def section(doc, rows): doc.h(2, f"Wrong-model null: {len(rows) - len(bad)} of {len(rows)} pass") doc.p("Each set run without --model and with five wrong models (nullmodel.json). A pair passes when " "(a) p.hkl, the p.mtz reflections and p.cif are identical to the no-model run's, (b) no " - "KEY= line outside MODEL VALIDATION moved, and (c) the model is recognised as wrong: " + "line of the report outside MODEL VALIDATION moved (KEY=, prose and WARNING: lines), and (c) the model is recognised as wrong: " "MODEL_VALIDATION CELL_MISMATCH (another lattice) and not placed by molecular replacement, or " "MODEL_FIT not ACCEPTED, not placed by molecular replacement either (MODEL_PLACEMENT not MR), " "no decision taken, CC_MODEL_CONFIRMED_TO_D_MIN NONE and R_free "