From dc71cb72992678a778e0d804bf2e75adcb7bd004 Mon Sep 17 00:00:00 2001 From: Filip Leonarski Date: Sun, 30 Aug 2026 11:35:45 +0200 Subject: [PATCH] rugnux: keep the P1 merge, so a wrong space group is recoverable A de-novo rotation run merges in P1 to search for the symmetry, adopts a group, and then overwrites that merge with the in-symmetry one. If the adopted group is wrong the user has no route back: every file the run wrote, and every statistic in them, is computed in the group that was assumed, so nothing in the output says the choice was wrong and the only way to a different answer is to process the images again. Merge the same integration once more in P1 after the run's own files are written, and put it beside them as _P1.mtz. From it the space group can be re-determined and the data re-merged, re-solved or re-refined in any subgroup - measured end to end on three crystals: POINTLESS reads the file, recovers the group, and AIMLESS re-merges it, reproducing rugnux's own merged intensities at CC 0.9965 where the two agree. On one of the three it recovered the deposited/XDS group where this run had under-called the screw axis. The merge is the full one - correction surfaces fitted, ice rings and near-tangential observations kept, whole resolution range - not the deliberately degraded merge the space-group search itself runs on, and it is what `rugnux --mode scale -S P1` produces from a _process.h5. That route already existed but needs a _process.h5, which a merging run does not write, so it only helped a user who had foreseen the problem. Every de-novo rotation run writes the file, including one whose search concluded P1 and where it therefore repeats the merged output byte for byte. Whether a file exists must not depend on what the pipeline decided: a script harvesting results would otherwise have to reproduce the search's decision to know whether to expect it, and a missing file would not separate "the run chose P1" from "the run failed". A user-fixed -S writes nothing, and that condition is not a pipeline decision. With a group fixed, prediction rejects that group's centring absences (IndexAndRefine.cpp:499-506), so those reflections are never integrated; a P1 merge built from such a run would be missing whole centring classes and would mislead rather than merely be smaller. Cost, median of five paired runs read off the log timestamps (the box is shared, so end-to-end wall time is noise): +0.32 s of 4.2 s, +2.18 s of 31.4 s, +0.19 s of 17.9 s, +0.66 s of 15.9 s - 1 to 8% of a run. The file is 1.6x the merged MTZ on a monoclinic crystal and 30x on a cubic one, where the merged MTZ is tiny; it is well under the unmerged export in every case measured, and 0.02 to 0.6% of the raw dataset. Rotation only for now: the stills merge re-fits per-image scales and per-reflection partialities onto the integration outcomes, and _unmerged.mtz is written from those afterwards, so on stills this extra merge alters a file that is the run's own output. Fixing that means running the cross-check below the unmerged export, which needs the merge lambda hoisted out of its block; deferred, since rotation is what the online pipeline processes. On rotation the merged .mtz, .cif, .hkl, _image.dat, _unmerged.mtz and _unmerged_partials.mtz are all byte-identical with and without this change, including on a run whose search returns P1. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01T3yNBXk4wKdMZy1ak2NY7f --- docs/CHANGELOG.md | 1 + docs/RUGNUX.md | 10 +++++++ rugnux/Rugnux.cpp | 61 +++++++++++++++++++++++++++++++++++++++++++ rugnux/Rugnux.h | 6 +++++ rugnux/rugnux_cli.cpp | 8 ++++++ 5 files changed, 86 insertions(+) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index e2c0875ec..49e476e91 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -1,6 +1,7 @@ # Changelog ## 1.0.0 ### 1.0.0-rc.166 +* Every rotation run that determines its own space group also writes `_P1.mtz`, the same observations merged in P1, so a wrong space group can be re-merged, re-solved or re-refined without processing the images again; `--no-p1-crosscheck` declines it, and a run given `-S` writes nothing because its space group's centring absences were never integrated. * `jfjoch_viewer` opens PILATUS miniCBF sweeps - naming any frame opens the whole sweep - and can run a processing job on one. * A detector whose stored image is mirrored in Y or mounted at a multiple of 90 degrees can be described as such, in the detector configuration or with `--detector-mirror-y` / `--detector-quarter-turns`, rather than having to be expressed as a detector rotation. * The rotation first pass refines twelve candidate lattices rather than four, so a correct cell that the pre-refinement ranking put fifth is still reached. diff --git a/docs/RUGNUX.md b/docs/RUGNUX.md index f8d7ccc55..9b810b87e 100644 --- a/docs/RUGNUX.md +++ b/docs/RUGNUX.md @@ -405,6 +405,16 @@ reusing them would hide the spot-finding settings from the lattice search. asks for it, alongside the merged files and with `--no-merge` too. See [The unmerged export](#the-unmerged-export) below. `--export-unmerged-partials` writes `_unmerged_partials.mtz`, one row per image, instead of summing. +- `_P1.mtz` — the **P1 cross-check dataset**: the same observations merged in P1 instead of the + space group the run determined, so a wrong call can be re-merged, re-solved or re-refined without + processing the images again. It is a full merge, not the degraded one the search itself runs on, and + it is what `rugnux --mode scale -S P1` would make from a `_process.h5`. **Every rotation run that + determines its own space group writes it** — including one that determined P1, where it simply + repeats the merged output — so a script harvesting results can always expect the file rather than + having to reproduce the search's decision to know whether it exists. It is **not** written when `-S` + fixed the space group: prediction then rejects that group's centring absences, so those reflections + were never integrated and a P1 merge of them would be missing whole classes of reflections. + `--no-p1-crosscheck` declines it. Stills are not covered yet. - `_report.txt` — the **results report**: what the run determined, in a form both a person and a beamline script can read. Always written, next to the files above. See [The results report](#the-results-report) below. diff --git a/rugnux/Rugnux.cpp b/rugnux/Rugnux.cpp index c10e62663..1b4274b76 100644 --- a/rugnux/Rugnux.cpp +++ b/rugnux/Rugnux.cpp @@ -3517,6 +3517,67 @@ ProcessResult Rugnux::RunPipeline(RugnuxObserver *observer, bool write_output, b // per-image arrays are only filled on the online per-image path). Sourced from the // partials, which carry the first-pass per-image scale. ScalingResult(indexer->GetIntegrationOutcome()).SaveToFile(config_.output_prefix); + + // P1 cross-check dataset. The group the files above are written in was chosen by the + // search, and if that choice is wrong nothing in them says so - every statistic was + // computed in the group that was assumed, and the only way back is to process the images + // again. Merging the same integration once more in P1 removes that: the user can re-merge + // it in any subgroup, run MR or refinement in it, or hand it to POINTLESS. It is a proper + // merge - correction surfaces fitted, ice rings and near-tangential observations kept - + // not the deliberately degraded one the search itself ran on, and it is the same file + // `rugnux --mode scale -S P1` makes from a _process.h5. Written after the merged output, + // so the answer of the run is already on disk in the group that was determined. + // + // Written on EVERY de-novo run, including one whose search concluded P1 and where the + // file therefore repeats the merged output. Whether a file exists must not depend on + // what the pipeline decided: a script harvesting results would otherwise have to + // reproduce the search's decision to know whether to expect it, and a missing file + // would not distinguish "the run chose P1" from "the run failed". + // + // A user-fixed space group (-S) is the one case that writes nothing, and the reason is + // not policy: with a group fixed, prediction rejects that group's centring absences and + // those reflections are never integrated (IndexAndRefine.cpp, `.centering = ...`). A P1 + // merge built from such a run would be missing whole centring classes - a misleading P1 + // dataset, not merely a smaller one. Do not "fix" this gap by writing the file anyway. + // + // Rotation only, for now. The stills merge re-fits each image's scale and each + // reflection's partiality onto the integration outcomes, and _unmerged.mtz is written + // from those afterwards - so on stills this merge changes a file that is the run's own + // output (measured: same length, different bytes). The fix is to run this below the + // unmerged export rather than here, which needs the merge lambda hoisted out of this + // block; deferred, as rotation is what the online pipeline processes. + if (config_.write_p1_crosscheck && search_space_group && is_rotation) { + const auto determined_sg = experiment_.GetSpaceGroupNumber(); + const gemmi::SpaceGroup *determined = gemmi::find_spacegroup_by_number( + static_cast(determined_sg.value_or(1))); + // scale_and_merge reports the error model of whatever it merged; this merge is not + // the run's answer, so put the determined group's model back afterwards. + const double em_isa = result.error_model_isa; + const double em_isa_asymptotic = result.error_model_isa_asymptotic; + const double em_a = result.error_model_a; + const double em_b = result.error_model_b; + // Both the merge and the MTZ read the group from the experiment, so it is set for + // the whole of it and restored after. + experiment_.SpaceGroupNumber(1); + const auto p1 = scale_and_merge("P1 cross-check", false); + const std::string path = config_.output_prefix + "_P1.mtz"; + WriteMtzReflections(p1.merged, *result.consensus_cell, experiment_, path); + experiment_.SpaceGroupNumber(determined_sg); + result.error_model_isa = em_isa; + result.error_model_isa_asymptotic = em_isa_asymptotic; + result.error_model_a = em_a; + result.error_model_b = em_b; + if (determined_sg.value_or(1) > 1) + logger.Info("P1 cross-check dataset written to {} ({} unique reflections): the " + "same observations merged in P1 instead of {}, so a wrong space group " + "can be recovered from without reprocessing. It is not the result of " + "this run.", path, p1.merged.size(), + determined ? determined->short_name() : "?"); + else + logger.Info("P1 cross-check dataset written to {} ({} unique reflections). This " + "run determined P1, so it repeats the merged output; it is written " + "anyway, so the file is there on every run.", path, p1.merged.size()); + } } } diff --git a/rugnux/Rugnux.h b/rugnux/Rugnux.h index 421ccc1b5..a65fb178f 100644 --- a/rugnux/Rugnux.h +++ b/rugnux/Rugnux.h @@ -151,6 +151,12 @@ struct ProcessConfig { // mmCIF, gzipped or not): compute R-free with an optimized bulk solvent and write 2Fo-Fc / // Fo-Fc maps (--model). std::string model_path; + + // Merge the same integration a second time in P1 and write it as _P1.mtz, so a + // space group the search got wrong stays recoverable without reprocessing (--no-p1-crosscheck + // declines it). Every de-novo rotation run writes it, including one that determined P1 itself; + // a user-fixed -S writes nothing, because its centring absences were never integrated. + bool write_p1_crosscheck = true; }; struct ProcessResult { diff --git a/rugnux/rugnux_cli.cpp b/rugnux/rugnux_cli.cpp index 2a99832a6..91d7069a4 100644 --- a/rugnux/rugnux_cli.cpp +++ b/rugnux/rugnux_cli.cpp @@ -170,6 +170,7 @@ void print_usage() { std::cout << " --model After merging, validate vs this atomic model: R-free, 2Fo-Fc/Fo-Fc maps, and - where the merge kept the Bijvoet split - an anomalous difference map with the strongest anomalous scatterers named by the atom they sit on. PDB or mmCIF, gzipped or not, recognised by content rather than by extension" << std::endl; std::cout << " --export-unmerged Write _unmerged.mtz, an unmerged MTZ (POINTLESS column layout) of the integrated observations, for aimless / pointless / careless. Off by default - the file is large, and a run that does not need it should not pay for it. On a rotation run the partials of each reflection are summed into one full, written at the batch its rocking curve is centred on, with the summed rocking-curve fraction in FRACTIONCALC; an event that caught less of its rocking curve than --min-partiality is not written, as in the merge. Intensities carry the Lorentz-polarization factor and nothing else: the partiality is not divided out and the per-image scale is not applied at all, since those programs scale the data themselves. Written in --mode mx and --mode scale, and with --no-merge" << std::endl; std::cout << " --export-unmerged-partials Write _unmerged_partials.mtz, the same observations with each partial as its own row (one batch per image) flagged for the reading program to sum, instead of summed here. Independent of --export-unmerged; both can be given" << std::endl; + std::cout << " --no-p1-crosscheck Do not write _P1.mtz. That file is the same observations merged in P1 instead of the space group the run determined, so a wrong space group can be re-merged, re-solved or re-refined without processing the images again. Every rotation run that determines its own space group writes it - including one that determined P1, where it repeats the merged output - so it is there to be harvested whatever the run decided. Not written when -S fixed the space group: that run never integrated the group's centring absences, so its P1 merge would be missing whole classes of reflections. It is a full merge, not the degraded one the search runs on, and it costs one extra merge" << std::endl; std::cout << std::endl; std::cout << " Integration" << std::endl; @@ -279,6 +280,7 @@ enum { OPT_CALIBRANT, OPT_CALIBRATION, OPT_NO_MERGE, + OPT_NO_P1_CROSSCHECK, OPT_POLARIZATION_CORRECTION, OPT_SOLID_ANGLE_CORRECTION, OPT_BEAM_X, @@ -317,6 +319,7 @@ static option long_options[] = { {"calibrant", required_argument, nullptr, OPT_CALIBRANT}, {"calibration", required_argument, nullptr, OPT_CALIBRATION}, {"no-merge", no_argument, nullptr, OPT_NO_MERGE}, + {"no-p1-crosscheck", no_argument, nullptr, OPT_NO_P1_CROSSCHECK}, {"scale-fulls", no_argument, nullptr, OPT_SCALE_FULLS}, {"no-scale-fulls", no_argument, nullptr, OPT_NO_SCALE_FULLS}, {"write-process-h5", no_argument, nullptr, OPT_WRITE_PROCESS_H5}, @@ -663,6 +666,7 @@ static int RunRugnux(int argc, char **argv) { std::optional rotation_indexing_range; std::optional min_indexed_spots; // --min-indexed-spots: spots on the lattice for a frame to count as indexed bool run_scaling = true; // merge is on by default; --no-merge turns it off + bool write_p1_crosscheck = true; // _P1.mtz is written by default; --no-p1-crosscheck declines it std::optional scale_fulls_arg; // --scale-fulls / --no-scale-fulls; default on for rot3d bool write_process_h5_flag = false; // --write-process-h5; also write _process.h5 when merging std::optional detect_ice_rings; // --detect-ice-rings[=on|off]; unset => use the dataset (file) value @@ -1043,6 +1047,9 @@ static int RunRugnux(int argc, char **argv) { case OPT_NO_MERGE: run_scaling = false; break; + case OPT_NO_P1_CROSSCHECK: + write_p1_crosscheck = false; + break; case OPT_SCALE_FULLS: scale_fulls_arg = true; break; @@ -2387,6 +2394,7 @@ static int RunRugnux(int argc, char **argv) { config.observation_dump_path = dump_observations; config.export_unmerged = export_unmerged; config.export_unmerged_partials = export_unmerged_partials; + config.write_p1_crosscheck = write_p1_crosscheck; config.model_path = model_pdb; // When merging, the merged reflections (.mtz/.cif) are the wanted output; skip the large // _process.h5 unless explicitly requested. Without merging, the _process.h5 is the only output.