diff --git a/common/CMakeLists.txt b/common/CMakeLists.txt index 59aaf46c3..8a4b2ca8d 100644 --- a/common/CMakeLists.txt +++ b/common/CMakeLists.txt @@ -3,18 +3,11 @@ FIND_PACKAGE(Git) -EXECUTE_PROCESS(COMMAND - "${GIT_EXECUTABLE}" describe --match=NeVeRmAtCh --always --abbrev=8 - WORKING_DIRECTORY "${CMAKE_SOURCE_DIR}" - OUTPUT_VARIABLE GIT_SHA1 - ERROR_QUIET OUTPUT_STRIP_TRAILING_WHITESPACE) - -# the date of the commit -EXECUTE_PROCESS(COMMAND - "${GIT_EXECUTABLE}" log -1 --format=%ad --date=local - WORKING_DIRECTORY "${CMAKE_SOURCE_DIR}" - OUTPUT_VARIABLE GIT_DATE - ERROR_QUIET OUTPUT_STRIP_TRAILING_WHITESPACE) +SET(GITINFO_SOURCE_DIR "${CMAKE_SOURCE_DIR}") +SET(GITINFO_TEMPLATE "${CMAKE_CURRENT_SOURCE_DIR}/GitInfo.cpp.in") +SET(GITINFO_OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/GitInfo.cpp") +SET(JFJOCH_BUILD_CXX_FLAGS "${CMAKE_CXX_FLAGS}") +INCLUDE("${CMAKE_CURRENT_SOURCE_DIR}/GitInfoStamp.cmake") set (THREADS_PREFER_PTHREAD_FLAG ON) find_package (Threads REQUIRED) @@ -23,10 +16,24 @@ MESSAGE(STATUS "Jungfraujoch git SHA1: ${GIT_SHA1}") MESSAGE(STATUS "Jungfraujoch git date: ${GIT_DATE}") MESSAGE(STATUS "Jungfraujoch version: ${JFJOCH_VERSION}") -CONFIGURE_FILE("${CMAKE_CURRENT_SOURCE_DIR}/GitInfo.cpp.in" "${CMAKE_CURRENT_BINARY_DIR}/GitInfo.cpp" @ONLY) +# Re-stamped on every build, so RUGNUX_GIT in the rugnux report (and every other consumer of +# jfjoch_git_sha1) names the commit that was actually compiled. +ADD_CUSTOM_TARGET(JFJochGitInfo + COMMAND "${CMAKE_COMMAND}" + "-DGIT_EXECUTABLE=${GIT_EXECUTABLE}" + "-DGITINFO_SOURCE_DIR=${CMAKE_SOURCE_DIR}" + "-DGITINFO_TEMPLATE=${CMAKE_CURRENT_SOURCE_DIR}/GitInfo.cpp.in" + "-DGITINFO_OUTPUT=${CMAKE_CURRENT_BINARY_DIR}/GitInfo.cpp" + "-DJFJOCH_VERSION=${JFJOCH_VERSION}" + "-DJFJOCH_BUILD_CXX_FLAGS=${CMAKE_CXX_FLAGS}" + -P "${CMAKE_CURRENT_SOURCE_DIR}/GitInfoStamp.cmake" + BYPRODUCTS "${CMAKE_CURRENT_BINARY_DIR}/GitInfo.cpp" + COMMENT "Stamping GitInfo.cpp from git HEAD" + VERBATIM) ADD_LIBRARY(JFJochVersion STATIC ${CMAKE_CURRENT_BINARY_DIR}/GitInfo.cpp GitInfo.h) +ADD_DEPENDENCIES(JFJochVersion JFJochGitInfo) set_target_properties(JFJochVersion PROPERTIES POSITION_INDEPENDENT_CODE ON ) diff --git a/common/GitInfo.cpp.in b/common/GitInfo.cpp.in index 98caf90e0..33868dd05 100644 --- a/common/GitInfo.cpp.in +++ b/common/GitInfo.cpp.in @@ -13,4 +13,8 @@ std::string jfjoch_git_date() { std::string jfjoch_version() { return "@JFJOCH_VERSION@"; -} \ No newline at end of file +} + +std::string jfjoch_build_cxx_flags() { + return "@JFJOCH_BUILD_CXX_FLAGS@"; +} diff --git a/common/GitInfo.h b/common/GitInfo.h index b61e79ec1..4951ef17a 100644 --- a/common/GitInfo.h +++ b/common/GitInfo.h @@ -8,3 +8,6 @@ std::string jfjoch_git_sha1(); std::string jfjoch_git_date(); std::string jfjoch_version(); +// CMAKE_CXX_FLAGS of the build - empty on a plain configure. Recorded because two builds of one +// commit can differ by flags alone, and -march moves the CPU-bound results. +std::string jfjoch_build_cxx_flags(); diff --git a/common/GitInfoStamp.cmake b/common/GitInfoStamp.cmake new file mode 100644 index 000000000..f2c9ce138 --- /dev/null +++ b/common/GitInfoStamp.cmake @@ -0,0 +1,20 @@ +# Stamps GitInfo.cpp with the SHA of HEAD as it is NOW. Run with `cmake -P` on every build (see the +# JFJochGitInfo target in CMakeLists.txt here), so the hash a binary reports is the commit it was +# built from, not the one the tree held when it was last configured - a configure-time stamp once +# reported a hash seventeen commits stale. configure_file leaves the output untouched when nothing +# changed, so this costs a recompile only when HEAD moves. Also included at configure time, so the +# file exists for the generator and the SHA prints with the configure output. +if (GIT_EXECUTABLE) + # --dirty: a build from uncommitted changes says so in the hash itself + EXECUTE_PROCESS(COMMAND + "${GIT_EXECUTABLE}" describe --match=NeVeRmAtCh --always --abbrev=8 --dirty + WORKING_DIRECTORY "${GITINFO_SOURCE_DIR}" + OUTPUT_VARIABLE GIT_SHA1 + ERROR_QUIET OUTPUT_STRIP_TRAILING_WHITESPACE) + EXECUTE_PROCESS(COMMAND + "${GIT_EXECUTABLE}" log -1 --format=%ad --date=local + WORKING_DIRECTORY "${GITINFO_SOURCE_DIR}" + OUTPUT_VARIABLE GIT_DATE + ERROR_QUIET OUTPUT_STRIP_TRAILING_WHITESPACE) +endif () +CONFIGURE_FILE("${GITINFO_TEMPLATE}" "${GITINFO_OUTPUT}" @ONLY) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index c0b76fab4..4adef1953 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -3,6 +3,9 @@ ### 1.0.0-rc.169 +* Every line of the rugnux report that is not `KEY= value` data and not blank now starts with `#` - warnings print as `# WARNING:` - so dropping the `#` lines leaves the data alone (`REPORT_VERSION= 14`). +* The rugnux report header records the exact build: the git commit (stamped at build time, so it cannot go stale; `-dirty` marks uncommitted changes), the compiler flags, and the download page of the release it came from. +* The rugnux report states at its foot who wrote the program and its terms of use: GPLv3, free for academic institutions and commercial companies alike. * The viewer's file manager lists CBF frames beside HDF5 datasets. * The viewer's image statistics state the collection time, detector distance, beam centre and wavelength as entries of their own rather than in a tooltip, and no longer report a B-factor. * The viewer plots spot count and background together on one plot, which is what a connection to a live source now shows by default. diff --git a/docs/RUGNUX_REPORT.md b/docs/RUGNUX_REPORT.md index 689ad95da..fdb0cc673 100644 --- a/docs/RUGNUX_REPORT.md +++ b/docs/RUGNUX_REPORT.md @@ -27,15 +27,17 @@ case — so a script branches on the exit status first and greps the report seco ## Format The model is XDS's `CORRECT.LP`: prose and tables a crystallographer reads top to bottom, with a -structure a script can consume without parsing prose. +structure a script can consume without parsing prose. Every line is one of three kinds — a +`KEY= value` data line, a `#` comment, or blank — so `grep -v '^#'` leaves the data alone +(since `REPORT_VERSION= 14`; before that, comment lines had no prefix). - **`KEY= value` assignment lines.** Every number worth extracting is one, so a consumer gets it with a single `grep '^ISA= '` and never has to read a sentence. Key names are stable. -- **Fixed-width tables** with a stable header row for anything that is genuinely tabular — the - resolution shells, the space-group candidates, the sweep-quality ranges. -- **`WARNING:` lines**, one per finding, in plain English: `WARNING: Frames 500-600 out of beam - (10.1 deg, scale 0.12 and CC 0.30 of the run, 2% scaled)`. `grep '^WARNING:'` finds every one. -- **Section banners** (`***…***` around a numbered title) delimiting the blocks. +- **`#` comment lines** — everything else: the prose, the section banners, and the fixed-width + tables (stable header row; the resolution shells, the space-group candidates, the sweep-quality + ranges). +- **`# WARNING:` lines**, one per finding, in plain English: `# WARNING: Frames 500-600 out of beam + (10.1 deg, scale 0.12 and CC 0.30 of the run, 2% scaled)`. `grep '^# WARNING:'` finds every one. **A quantity the run did not measure writes no key at all**, and the fixed-width tables print `-` in its place. There is one rule and no placeholders — no `nan`, and no `0.0%` that reads as a measured @@ -46,10 +48,12 @@ not `-A` was given, but where no Bijvoet pair could be split in both hands — a `-A`, or too few pairs — the quantity does not exist and the key is absent; `COMPLETENESS=`, `MULTIPLICITY=`, `I_OVER_SIGMA=`, `R_MEAS=`, `CC_HALF=` and `WILSON_B=` follow the same rule. -The last block before `END OF REPORT` is the acknowledgement — the crystallographic methods rugnux -implements were published by the X-ray research community and the program is built on open-source -software from many projects, both credited in `ACKNOWLEDGEMENT.md` beside `LICENSE` and -`THIRD_PARTY_NOTICES.md` in the installed package. rugnux prints the same lines at startup. +The last blocks before `END OF REPORT` are the authorship and the acknowledgement: who wrote +rugnux, its licence (GPLv3 — free to use for academic institutions and commercial companies alike) +and where releases are published, then the credit to the X-ray research community whose methods +rugnux implements and the open-source projects it is built on — both credited in +`ACKNOWLEDGEMENT.md` beside `LICENSE` and `THIRD_PARTY_NOTICES.md` in the installed package. +rugnux prints the acknowledgement at startup as well. `REPORT_VERSION=` is the format's own version. Key names, table columns and the reason vocabulary below are an interface other software may depend on: they do not change without that number moving. @@ -57,7 +61,14 @@ Adding a key does not move it — a consumer that greps for what it needs is una line. The header block above section 1 records **how the result was produced**: `RUGNUX_VERSION=` and -`RUGNUX_GIT=`, `DATE=`, `INPUT_FILE=` and `OUTPUT_PREFIX=`, plus +`RUGNUX_DOWNLOAD=` (the release page of exactly that version), `RUGNUX_GIT=`, `BUILD_CXX_FLAGS=`, +`DATE=`, `INPUT_FILE=` and `OUTPUT_PREFIX=`, plus + +- **`RUGNUX_GIT=`** — the commit the binary was built from, stamped at build time so it cannot go + stale in a reconfigured tree; a `-dirty` suffix marks a build from uncommitted changes. +- **`BUILD_CXX_FLAGS=`** — the compiler flags of the build (`NONE` for a plain configure). Two + builds of one commit can differ by flags alone, and `-march` moves the CPU-bound results, so a + comparison of two reports starts here. - **`COMMAND_LINE=`** — the invocation as one shell-ready line, arguments containing spaces quoted. - **`WALL_TIME=`** — the whole invocation in seconds. It covers everything the process did, opening diff --git a/image_analysis/scale_merge/SearchSpaceGroup.cpp b/image_analysis/scale_merge/SearchSpaceGroup.cpp index 68da73a0b..58cefc2e2 100644 --- a/image_analysis/scale_merge/SearchSpaceGroup.cpp +++ b/image_analysis/scale_merge/SearchSpaceGroup.cpp @@ -2428,7 +2428,7 @@ std::string FinalistLedgerToText(const SearchSpaceGroupResult& result) { // How to read it. Every sentence here is something the numbers above have been measured to NOT // support, and each was a wrong reading somebody actually made. - os << "\n READ IT AS AN ADMISSION TEST, NOT A RANKING: R/best rises with point-group order by\n" + os << "\n THE TABLE IS AN ADMISSION TEST, NOT A RANKING: R/best rises with point-group order by\n" " construction - a larger group adds more operators - so the smallest non-trivial subgroup\n" " very often has the lowest R/best in the table. Sorting these rows and taking the minimum\n" " would demote nearly every genuinely high-symmetry crystal. The question the table answers\n" @@ -2439,7 +2439,7 @@ std::string FinalistLedgerToText(const SearchSpaceGroupResult& result) { " DIAGNOSTIC ONLY - on the calibration set chi2/best and b/par put their LARGEST value on a\n" " GENUINE crystal, with both known false cases inside the genuine range. A ratio to the\n" " PARENT is contaminated whenever the parent is itself false, which is exactly the case\n" - " these columns would have to catch. Do not read them as a ranking or a second opinion.\n" + " these columns would have to catch. They are neither a ranking nor a second opinion.\n" "\n BLIND SPOTS. This table is folded from merged intensities, so it can only see a\n" " hypothesis that DEGRADES A MERGE. It is structurally blind to the other two ways a group\n" " is over-called: a screw-axis over-call is in the same Laue class and folds a byte-identical\n" diff --git a/image_analysis/scale_merge/TranslationalNCS.cpp b/image_analysis/scale_merge/TranslationalNCS.cpp index eb08a926e..9c1932eba 100644 --- a/image_analysis/scale_merge/TranslationalNCS.cpp +++ b/image_analysis/scale_merge/TranslationalNCS.cpp @@ -605,8 +605,9 @@ std::string TranslationalNCSToText(const TranslationalNCSResult &result) { if (result.near_extinct_class) os << " => The class this vector suppresses is not weak but very nearly EXTINCT, which is\n" << " what a lattice translation of a smaller cell looks like measured in a larger one.\n" - << " Treat the reported cell as a supercell candidate rather than a settled result: a\n" - << " cell with this translation as a lattice vector is the rival to check.\n"; + << " The reported cell is therefore a supercell candidate rather than a settled\n" + << " result, and the rival hypothesis is a cell with this translation as a lattice\n" + << " vector.\n"; else if (result.commensurate) os << " => The vector is a 1/" << result.commensurate_denominator << " translation of the cell, so the crystal is pseudo-centred. The cell itself is not in\n" diff --git a/image_analysis/scale_merge/TwinningAnalysis.cpp b/image_analysis/scale_merge/TwinningAnalysis.cpp index 9a8183d15..0a16c3cde 100644 --- a/image_analysis/scale_merge/TwinningAnalysis.cpp +++ b/image_analysis/scale_merge/TwinningAnalysis.cpp @@ -313,7 +313,7 @@ std::string TwinningAnalysisToText(const TwinningAnalysisResult& result) { if (result.twinning_suspected && result.estimated_twin_fraction > 0.01) os << " => Twinning suspected (estimated twin fraction ~" << result.estimated_twin_fraction << "). Statistics flag the presence of twinning, not\n" - << " the twin law; confirm with a dedicated twin-law analysis.\n"; + << " the twin law; the law itself is a question for a dedicated twin-law analysis.\n"; else if (result.twinning_suspected) // The fraction is derived from the second moment alone, so a twin called by the L-test on a // crystal whose second moment sits at or above its untwinned value has no fraction to quote - @@ -323,25 +323,26 @@ std::string TwinningAnalysisToText(const TwinningAnalysisResult& result) { << result.second_moment << ", at or\n" << " above its untwinned 2.00), so no twin fraction is quoted" << (result.l_test_tncs_step_restricted - ? " - and this crystal's translational pseudo-symmetry is a known reason for\n" - " the second moment to run high, so the L-test is the one to believe here.\n" + ? " - and this\n" + " crystal's translational pseudo-symmetry is a known reason for the second\n" + " moment to run high, so the L-test carries the verdict here.\n" : ".\n") - << " Statistics flag the presence of twinning, not the twin law; confirm with a\n" - << " dedicated twin-law analysis.\n"; + << " Statistics flag the presence of twinning, not the twin law; the law itself is\n" + << " a question for a dedicated twin-law analysis.\n"; else if (!result.merohedral_twinning_possible && result.laue_class_was_chosen_by_promotion) os << " => Cannot rule out twinning from these numbers: the Laue class is holohedral, so no\n" << " merohedral twin law exists WITHIN it - but this Laue class was chosen by the\n" << " space-group search itself, and promoting into a twin's holohedry is precisely what\n" - << " a merohedral twin looks like. Judge the twinning from the subgroup statistics\n" - << " reported by the search, not from these.\n"; + << " a merohedral twin looks like. The subgroup statistics reported by the search,\n" + << " not these numbers, are where the twinning is decided.\n"; else if (!result.merohedral_twinning_possible) os << " => No twinning: the Laue class is holohedral, so no merohedral twin law exists\n" << " (any <|L|> below 0.5 here is a statistical artefact, not twinning).\n"; else if (result.l_test_contaminated_by_tncs) os << " => No twinning indicated by the second moment. The L-test, which is normally the\n" << " stronger of the two, could not be read on this crystal (see above), so this is a\n" - << " weaker statement than usual - if the space-group search reported subgroup\n" - << " statistics, judge the twinning from those.\n"; + << " weaker statement than usual - where the space-group search reported subgroup\n" + << " statistics, those are the stronger evidence.\n"; else os << " => No twinning indicated.\n"; return os.str(); diff --git a/rugnux/ResultReport.cpp b/rugnux/ResultReport.cpp index bf727e768..116f1a4ab 100644 --- a/rugnux/ResultReport.cpp +++ b/rugnux/ResultReport.cpp @@ -34,7 +34,11 @@ namespace { // whether choosing the partner reflections differently could repair it. // 13: CC_MODEL_* in section 5 - the correlation of the merged intensities with the placed model, // by resolution shell, which only a run given a model can report. - constexpr int REPORT_VERSION = 7; + // 14: every line that is not `KEY= value` and not blank starts with `#`, warnings included + // (`# WARNING:`); the header gains RUGNUX_DOWNLOAD and BUILD_CXX_FLAGS, and RUGNUX_GIT is + // the full build-time hash. Versions 8-13 shipped with this constant still reading 7 by + // mistake, which is why the number resumes at 14. + constexpr int REPORT_VERSION = 14; const char *BANNER = " ******************************************************************************"; @@ -200,15 +204,26 @@ ReportDocument BuildReportDocument(const std::string &output_prefix, // ------------------------------------------------------------------ header block { ReportSection h; - Add(h, Prose(" What this run determined, written next to its other output. The `KEY= value` lines\n" - " and the tables below are a stable interface - a script greps them, and REPORT_VERSION\n" - " says when that interface last changed. --developer adds the pipeline internals and the\n" - " long explanations; leaving it off relocates them, it does not lose them.")); + Add(h, Prose(" What this run determined, written next to its other output. Every line is one of\n" + " three kinds - a `KEY= value` data line, a `#` comment, or blank - so dropping the\n" + " `#` lines leaves the data alone. The keys and tables are a stable interface, and\n" + " REPORT_VERSION says when that interface last changed. --developer adds the pipeline\n" + " internals and the long explanations; leaving it off relocates them, it does not lose\n" + " them.")); Add(h, Blank()); Add(h, KeyInt("REPORT_VERSION", REPORT_VERSION)); Add(h, KeyText("RUGNUX_VERSION", jfjoch_version())); + // The release page of exactly this version; the tag on a release is the version string. + Add(h, KeyText("RUGNUX_DOWNLOAD", + "https://gitea.psi.ch/mx/Jungfraujoch/releases/tag/" + jfjoch_version())); + // The commit the binary was built from - stamped at BUILD time, not configure time, and a + // "-dirty" suffix marks uncommitted changes - and the compiler flags it was built with. + // Both are provenance the version string does not carry: two builds of one commit can + // differ by flags alone, and -march moves the CPU-bound results. if (!jfjoch_git_sha1().empty()) - Add(h, KeyText("RUGNUX_GIT", jfjoch_git_sha1().substr(0, 6) + " " + jfjoch_git_date())); + Add(h, KeyText("RUGNUX_GIT", jfjoch_git_sha1() + " " + jfjoch_git_date())); + Add(h, KeyText("BUILD_CXX_FLAGS", + jfjoch_build_cxx_flags().empty() ? "NONE" : jfjoch_build_cxx_flags())); Add(h, KeyText("DATE", time_UTC(std::chrono::system_clock::now()))); Add(h, KeyText("INPUT_FILE", input_file)); Add(h, KeyText("OUTPUT_PREFIX", output_prefix)); @@ -283,8 +298,8 @@ ReportDocument BuildReportDocument(const std::string &output_prefix, if (medium == FlightPathMedium::Air && std::fabs(dB) >= 2.0) Warn(doc, PathologyCode::FLIGHT_PATH, fmt::format( "an AIR flight path was assumed, which is worth {:.1f} A^2 of Wilson B here; " - "no file states the medium, so if this station used a helium cone or an " - "evacuated flight tube, re-run with --flight-path helium (or vacuum)", dB)); + "no file states the medium, so a station with a helium cone or an evacuated " + "flight tube needs a re-run with --flight-path helium (or vacuum)", dB)); } // The same geometry once more as the object jfjoch_broker takes it in - the four required @@ -342,8 +357,8 @@ ReportDocument BuildReportDocument(const std::string &output_prefix, " is the DIRECT_BEAM above and not the split between the two; what survives that alias\n" " grows as the square of the scattering angle, and on a short-distance long-wavelength\n" " sweep it is large enough that the fit reproduces a separately measured tilt to a few\n" - " hundredths of a degree. Where 2theta is ordinary, compare the MEDIAN of this value over\n" - " several crystals collected on the same detector rather than one run's: that does track\n" + " hundredths of a degree. Where 2theta is ordinary, the MEDIAN of this value over\n" + " several crystals collected on the same detector - not one run's - does track\n" " a calibration well enough to show up a placeholder or a stale tilt in the file, and it\n" " does not replace the calibration.\n" " rot3 is omitted because a rotation about the beam is an exact null of this experiment\n" @@ -477,8 +492,9 @@ ReportDocument BuildReportDocument(const std::string &output_prefix, Add(s, Prose(line)); if (enantiomorphic && !alts.empty() && alts.find('|') == std::string::npos) Add(s, Prose(" The pair differs only in the hand of the screw axis, which merged intensities\n" - " cannot name. Try both in molecular replacement and keep whichever refines -\n" - " that is the normal outcome for a chiral group, not a failure of the run.")); + " cannot name. Molecular replacement settles it: one of the two refines and the\n" + " other does not - the normal outcome for a chiral group, not a failure of the\n" + " run.")); if (refused) Add(s, Prose(fmt::format( " A promotion to {} was refused: {}.\n" @@ -678,7 +694,8 @@ ReportDocument BuildReportDocument(const std::string &output_prefix, o.total_observations); Warn(doc, PathologyCode::UNUSABLE_MERGE, "These merged data are not usable as they stand: " + why - + ". Do not refine against the written reflections without establishing why"); + + ". The written reflection files carry the same problem, and refinement against " + "them is meaningless until the cause is established"); } // Completeness measured where the signal is, not over the generous cut - so a // corner-limited detector on good data does not fire it and a genuinely thin dataset does. @@ -687,12 +704,12 @@ ReportDocument BuildReportDocument(const std::string &output_prefix, Warn(doc, PathologyCode::LOW_COMPLETENESS, fmt::format( "Only {:.1f}% of the unique reflections to {:.2f} A were measured - the merge is " "missing a large part of reciprocal space, which no amount of multiplicity in what " - "was measured makes up for. Collect a wider sweep, or a second one about a " - "different axis", completeness_fit.percent, completeness_fit.d_min)); + "was measured makes up for. A wider sweep, or a second one about a different " + "axis, is what fills the gap", completeness_fit.percent, completeness_fit.d_min)); if (!fit_suppressed.empty() && !unusable_merge) Warn(doc, PathologyCode::RESOLUTION_FIT, "No resolution could be fitted: " + fit_suppressed - + ". Read the shell table rather than quoting a single number"); + + ". The shell table, not a single number, says where the signal stops"); } doc.sections.push_back(std::move(s)); } @@ -797,9 +814,9 @@ ReportDocument BuildReportDocument(const std::string &output_prefix, Add(s, Prose(fmt::format( " Translational pseudo-symmetry: INDICATED. The native Patterson has an off-origin peak\n" " at {:.1f}% of the origin at ({:.3f}, {:.3f}, {:.3f}), {:.1f} A, and the intensities\n" - " modulate {:.1f}x along that vector. Declare it to the molecular-replacement program:\n" - " the modulation is not in the search model, and without it the search can fail on data\n" - " that are otherwise good.", + " modulate {:.1f}x along that vector. The molecular-replacement program needs the vector\n" + " declared: the modulation is not in the search model, and without it the search can\n" + " fail on data that are otherwise good.", t.peak_percent, t.vector_frac[0], t.vector_frac[1], t.vector_frac[2], t.vector_length_A, t.modulation))); else if (t.modulation_measured) @@ -819,8 +836,8 @@ ReportDocument BuildReportDocument(const std::string &output_prefix, Add(s, ProseDefaultOnly( "\n The class this translation suppresses is not weak but very nearly EXTINCT, which is what\n" " a lattice translation of a smaller cell looks like when it is measured in a larger one.\n" - " Treat the reported cell as a supercell candidate rather than a settled result: a cell\n" - " with this translation as a lattice vector is the rival to check.")); + " The reported cell is therefore a supercell candidate rather than a settled result,\n" + " and the rival hypothesis is a cell with this translation as a lattice vector.")); if (!t.undeclared_lattice_translations.empty()) { const auto &v = t.undeclared_lattice_translations.front(); @@ -845,8 +862,8 @@ ReportDocument BuildReportDocument(const std::string &output_prefix, if (t.detected) Warn(doc, PathologyCode::PSEUDO_TRANSLATION, fmt::format( "Translational pseudo-symmetry: Patterson off-origin peak {:.1f}% of the origin at " - "({:.3f}, {:.3f}, {:.3f}), {:.1f} A - declare it to the molecular-replacement program, " - "which will otherwise be searching against a modulation it does not model", + "({:.3f}, {:.3f}, {:.3f}), {:.1f} A - the molecular-replacement program needs the " + "vector declared, or it will be searching against a modulation it does not model", t.peak_percent, t.vector_frac[0], t.vector_frac[1], t.vector_frac[2], t.vector_length_A)); if (!t.undeclared_lattice_translations.empty()) @@ -860,7 +877,7 @@ ReportDocument BuildReportDocument(const std::string &output_prefix, if (t.detected && t.near_extinct_class) Warn(doc, PathologyCode::LATTICE_TRANSLATION, fmt::format( "The pseudo-translation at {:.1f} A suppresses its phase class almost completely, so the " - "reported cell may be a supercell - check the indexing before treating the cell as settled", + "reported cell may be a supercell - it is not settled until the indexing has been re-examined", t.vector_length_A)); } } @@ -900,7 +917,8 @@ ReportDocument BuildReportDocument(const std::string &output_prefix, Add(s, Prose(fmt::format( " Twinning: INDICATED. <|L|> is {:.3f}, where an untwinned crystal reads about 0.5 and\n" " a perfect twin about 0.375. The estimated fraction {:.2f} is derived from the second\n" - " moment alone and the L-test may imply a larger one. Refine with a twin law.", + " moment alone and the L-test may imply a larger one. Refinement against these data\n" + " needs a twin law.", tw.mean_abs_l, tw.estimated_twin_fraction))); else if (tw.l_test_contaminated_by_tncs) // <|L|> is the number this sentence would otherwise rest on, and on this crystal it is @@ -923,11 +941,13 @@ ReportDocument BuildReportDocument(const std::string &output_prefix, " above are computed in the Laue class this run ADOPTED, so if the search promoted the\n" " point group they can only report that no twin law exists inside the class it chose -\n" " and a twin is precisely what would have caused that promotion. Where the two pairs\n" - " disagree, believe the _BEFORE_SEARCH one about whether the crystal is twinned.", true)); + " disagree, the _BEFORE_SEARCH pair is the one that answers whether the crystal is\n" + " twinned.", true)); if (tw.twinning_suspected) Warn(doc, PathologyCode::TWINNING, fmt::format( "Twinning is indicated (<|L|> = {:.3f}, /^2 = {:.3f}, estimated twin " - "fraction {:.2f} from the second moment) - refine against the merged data with care", + "fraction {:.2f} from the second moment) - refinement against the merged data " + "needs a twin law", tw.mean_abs_l, tw.second_moment, tw.estimated_twin_fraction)); } @@ -946,7 +966,7 @@ ReportDocument BuildReportDocument(const std::string &output_prefix, Add(s, Blank()); if (std::isfinite(db)) Add(s, Prose(fmt::format( - " Radiation damage: the relative B rises by {:.2f} A^2 over the sweep{}.", + " Radiation damage: the relative B changes by {:+.2f} A^2 over the sweep{}.", db, db < 2.0 ? ", which is negligible" : ""))); Add(s, Prose("\n" + result.radiation_damage_text, true)); } @@ -1041,10 +1061,18 @@ ReportDocument BuildReportDocument(const std::string &output_prefix, Add(s, KeyEnum("ANISOTROPY_VERDICT", AnisotropyVerdictCode(an.verdict), {"DETECTED", "NOT_DETECTED", "CANNOT_DETERMINE"})); Add(s, KeyReal("ANISOTROPY_DELTA_B", an.delta_b, "{:.2f}")); - Add(s, KeyText("ANISOTROPY_D_MIN_PRINCIPAL", - fmt::format("{:.2f} {:.2f} {:.2f}", an.d_min_axis[0], an.d_min_axis[1], - an.d_min_axis[2]))); - Add(s, KeyReal("ANISOTROPY_D_MIN_SPREAD", an.d_min_spread, "{:.2f}")); + // The one rule again: a direction the fit could not limit prints "-", and a triplet + // with no measured direction at all writes no key - never the word "nan". + if (std::isfinite(an.d_min_axis[0]) || std::isfinite(an.d_min_axis[1]) + || std::isfinite(an.d_min_axis[2])) { + std::string principal; + for (int i = 0; i < 3; ++i) + principal += (i ? " " : "") + (std::isfinite(an.d_min_axis[i]) + ? fmt::format("{:.2f}", an.d_min_axis[i]) : std::string("-")); + Add(s, KeyText("ANISOTROPY_D_MIN_PRINCIPAL", principal)); + } + if (std::isfinite(an.d_min_spread)) + Add(s, KeyReal("ANISOTROPY_D_MIN_SPREAD", an.d_min_spread, "{:.2f}")); // The gate's internals. Correct, and none of it changes what a user does next; the verdict // key above already carries the decision they were computed to make. @@ -1303,9 +1331,9 @@ ReportDocument BuildReportDocument(const std::string &output_prefix, } Add(s, std::move(e)); Add(s, Prose("\n" - " Read it in ONE direction only. A shell whose correlation is significantly above zero\n" - " carries signal, because a model cannot agree by accident with measurements it was\n" - " never fitted to, so CC_MODEL_CONFIRMED_TO_D_MIN is a lower bound on the useful\n" + " The table reads in ONE direction only. A shell whose correlation is significantly\n" + " above zero carries signal, because a model cannot agree by accident with measurements\n" + " it was never fitted to, so CC_MODEL_CONFIRMED_TO_D_MIN is a lower bound on the useful\n" " resolution and an argument for keeping MORE data. A shell whose correlation is near\n" " zero says nothing about the data: the model may be incomplete, in the wrong hand or\n" " simply wrong for this crystal, and cutting data on it would be cutting because the\n" @@ -1414,14 +1442,16 @@ ReportDocument BuildReportDocument(const std::string &output_prefix, doc.verdict_text += fmt::format(" (or {}, which these data cannot separate from it)", alt); doc.verdict_text += "."; if (unusable_merge) - doc.verdict_text += " The merged data carry no usable signal - see the warnings before" - " using any of the written files."; + doc.verdict_text += " The merged data carry no usable signal - the warnings say why," + " and the written files carry the same problem."; else if (!doc.pathology_flags.empty()) { std::string f; for (const auto &c : doc.pathology_flags) f += (f.empty() ? "" : ", ") + c; - doc.verdict_text += fmt::format(" {} condition(s) need attention: {}.", - doc.pathology_flags.size(), f); + doc.verdict_text += fmt::format(" {} condition{} attention: {}.", + doc.pathology_flags.size(), + doc.pathology_flags.size() == 1 ? " needs" : "s need", + f); } else { doc.verdict_text += " No warnings."; } @@ -1540,7 +1570,28 @@ ReportDocument BuildReportDocument(const std::string &output_prefix, // text file looks, which is what makes a second rendering a second function rather than a fork. std::string RenderReportText(const ReportDocument &doc, bool developer) { std::ostringstream os; - os << BANNER << "\n RUGNUX PROCESSING REPORT\n" << BANNER << "\n\n"; + // Everything that is not a `KEY= value` line and not blank is written as a `#` comment, so a + // consumer that drops the `#` lines is left with the data alone. Applied here, on whole + // rendered blocks, so no emitter can forget it. + const auto comment = [&os](const std::string &text) { + size_t start = 0; + while (true) { + const size_t end = text.find('\n', start); + const std::string line = text.substr( + start, end == std::string::npos ? std::string::npos : end - start); + if (line.empty()) + os << "\n"; + else + os << (line[0] == ' ' ? "#" : "# ") << line << "\n"; + if (end == std::string::npos) + break; + start = end + 1; + } + }; + comment(BANNER); + comment(" RUGNUX PROCESSING REPORT"); + comment(BANNER); + os << "\n"; for (const auto §ion : doc.sections) { if (section.developer_only && !developer) @@ -1552,8 +1603,13 @@ std::string RenderReportText(const ReportDocument &doc, bool developer) { }); if (!any) continue; - if (!section.title.empty()) - os << "\n" << BANNER << "\n " << section.title << "\n" << BANNER << "\n\n"; + if (!section.title.empty()) { + os << "\n"; + comment(BANNER); + comment(" " + section.title); + comment(BANNER); + os << "\n"; + } for (const auto &e : section.entries) { if ((e.developer_only && !developer) || (e.default_only && developer)) @@ -1563,15 +1619,15 @@ std::string RenderReportText(const ReportDocument &doc, bool developer) { os << e.key << "= " << e.value.text << "\n"; break; case ReportEntry::Kind::Prose: - os << e.prose << "\n"; + comment(e.prose); break; case ReportEntry::Kind::Blank: os << "\n"; break; case ReportEntry::Kind::Table: - os << e.table.text_header << "\n"; + comment(e.table.text_header); for (const auto &r : e.table.text_rows) - os << r << "\n"; + comment(r); break; } } @@ -1583,16 +1639,22 @@ std::string RenderReportText(const ReportDocument &doc, bool developer) { for (const auto &w : doc.warnings) { if (w.developer_only && !developer) continue; - os << "WARNING: " << w.text << "\n"; + comment("WARNING: " + w.text); ++shown; } if (shown == 0) - os << " (none)\n"; + comment(" (none)"); } } - os << "\n" << RUGNUX_ACKNOWLEDGEMENT << "\n"; - os << "\n" << BANNER << "\n END OF REPORT\n" << BANNER << "\n"; + os << "\n"; + comment(RUGNUX_AUTHORSHIP); + os << "\n"; + comment(RUGNUX_ACKNOWLEDGEMENT); + os << "\n"; + comment(BANNER); + comment(" END OF REPORT"); + comment(BANNER); return os.str(); } diff --git a/rugnux/ResultReport.h b/rugnux/ResultReport.h index 220f8400a..f20c015b8 100644 --- a/rugnux/ResultReport.h +++ b/rugnux/ResultReport.h @@ -34,6 +34,17 @@ struct RunProvenance { bool developer = false; }; +// Authorship, licence and terms of use, written at the foot of every report. The facts are the +// repository's: the copyright headers name the author, LICENSE is GPLv3, and the sentence about +// academic and commercial use restates what that licence already grants - said here because a +// report travels to readers who will never open a licence file. +inline constexpr const char *RUGNUX_AUTHORSHIP = + "rugnux is written by Filip Leonarski at the Paul Scherrer Institute.\n" + "Copyright (C) 2019-2026 Paul Scherrer Institute. It is free software under the\n" + "GNU General Public License v3 (see the LICENSE file): academic institutions and\n" + "commercial companies alike are free to use it.\n" + "Releases: https://gitea.psi.ch/mx/Jungfraujoch/releases"; + // The acknowledgement, printed by the CLI at startup and written at the foot of every report, so a // result carries it wherever it travels. The methods are the field's, not this program's, and the // program stands on other people's open-source code; the files named here are installed beside the diff --git a/rugnux/Rugnux.cpp b/rugnux/Rugnux.cpp index 0ac770c0a..b4de47b11 100644 --- a/rugnux/Rugnux.cpp +++ b/rugnux/Rugnux.cpp @@ -4541,8 +4541,9 @@ ProcessResult Rugnux::RunPipeline(RugnuxObserver *observer, bool write_output, b "Space group is AMBIGUOUS between {} and {} (point groups {} and {}, the same " "order): the merge of all observations and the merge of only the well-measured " "ones each support one of them, and neither is the higher symmetry, so the data " - "do not decide. Processing continues in {} (the all-observation choice) - try " - "BOTH in molecular replacement, or re-run with -S {} to force the other one.", + "do not decide. Processing continues in {} (the all-observation choice); " + "molecular replacement in both is what settles it, and a re-run with -S {} " + "forces the other one.", a, b, alt.point_group_hm, sg_search.point_group_hm, a, b); logger.Warning("{}", msg); stats_text << " !! " << msg << "\n\n"; @@ -5297,9 +5298,9 @@ ProcessResult Rugnux::RunPipeline(RugnuxObserver *observer, bool write_output, b "measured point group cannot map that part of the sweep's blind cone " "onto measured territory, however long the sweep runs. The loss is a " "coherent cap about the spindle direction rather than a scatter, so " - "overall completeness may still look reasonable - check it near the " - "spindle. A second sweep about a different axis, or re-mounting, " - "recovers it.", + "overall completeness may still look reasonable while the gap sits at " + "the spindle direction. A second sweep about a different axis, or " + "re-mounting, recovers it.", 100.0 * loss->lost_unique_fraction); logger.Warning("{}", msg); stats_text << " !! " << msg << "\n\n"; @@ -5338,7 +5339,7 @@ ProcessResult Rugnux::RunPipeline(RugnuxObserver *observer, bool write_output, b "Indexing ambiguity: this cell / space group admits alternative indexing " "(reindex operator(s): {}). {} are indexed in one hand at random, and rugnux can only " "break this against an external reference. WITHOUT one the merge mixes the hands and " - "CC1/2 is degraded - supply a reference MTZ (-z) or a model ({}) to resolve it.", + "CC1/2 is degraded; a reference MTZ (-z) or a model ({}) resolves it.", laws, experiment_.IsRotationIndexing() ? "Lattices" : "Serial-stills crystals", experiment_.IsRotationIndexing() ? "--model" : "--model, which needs -C and -S here"); logger.Warning("{}", msg); diff --git a/tests/ResultReportTest.cpp b/tests/ResultReportTest.cpp index e93069571..b0ab54561 100644 --- a/tests/ResultReportTest.cpp +++ b/tests/ResultReportTest.cpp @@ -70,7 +70,7 @@ TEST_CASE("ResultReport_Render", "[Diagnostics]") { // The stable keys a consumer greps for. The version is pinned on purpose: a key added to the // report is a contract change, and this line is where it has to be acknowledged. - CHECK(text.find("\nREPORT_VERSION= 7\n") != std::string::npos); + CHECK(text.find("\nREPORT_VERSION= 14\n") != std::string::npos); // SOHNCKE_SPACE_GROUP names the best group a chiral crystal could have, and it comes from the // space-group SEARCH. This fixture is given its group rather than searching for one, so there is // no Sohncke candidate to name and the key is absent - which is the honest behaviour and the @@ -113,8 +113,8 @@ TEST_CASE("ResultReport_Render", "[Diagnostics]") { CHECK(text.find("\nVERDICT= WARNINGS\n") != std::string::npos); CHECK(text.find("\nPATHOLOGY_FLAGS= SWEEP_GAPS\n") != std::string::npos); CHECK(text.find("VERDICT=") < text.find("SWEEP_QUALITY_COUNT=")); - CHECK(text.find("\nWARNING: Frames 100-149 out of beam (5.0 deg,") != std::string::npos); - CHECK(text.find("\nWARNING: Frames 400-499 radiation damage (10.0 deg,") != std::string::npos); + CHECK(text.find("\n# WARNING: Frames 100-149 out of beam (5.0 deg,") != std::string::npos); + CHECK(text.find("\n# WARNING: Frames 400-499 radiation damage (10.0 deg,") != std::string::npos); } TEST_CASE("ResultReport_RenderEmpty", "[Diagnostics]") { @@ -363,7 +363,7 @@ TEST_CASE("ResultReport_ModelValidationSection", "[Diagnostics]") { CHECK(cc_text.find("\nCC_MODEL_CONFIRMED_TO_D_MIN= 2.10\n") != std::string::npos); CHECK(cc_text.find("D_MIN CC_MODEL N SIGMA") != std::string::npos); CHECK(cc_text.find(" 1.80 0.0412 2110 +1.9") != std::string::npos); - CHECK(cc_text.find("Read it in ONE direction only") != std::string::npos); + CHECK(cc_text.find("The table reads in ONE direction only") != std::string::npos); // Nothing significant anywhere: the key still has to be written, saying so. ModelValidationResult no_signal = with_cc; @@ -590,7 +590,7 @@ TEST_CASE("ResultReport_TranslationalNCS", "[Diagnostics]") { { const auto text = RenderResultReport("prefix", "in.h5", x, result); CHECK(text.find("\nTNCS_DETECTED= NOT_MEASURED\n") != std::string::npos); - CHECK(text.find("not a\n statement that this crystal has none") != std::string::npos); + CHECK(text.find("not a\n# statement that this crystal has none") != std::string::npos); } result.tncs.measurable = true;