v1.0.0-rc.165 #75

Merged

leonarski_f merged 22 commits from rc165 into main 2026-08-27 22:16:54 +02:00

Conversation 0 Commits 22 Files Changed 347

+15397 -332

leonarski_f commented 2026-08-27 16:42:15 +02:00

Owner

Copy Link

Copy Source

• rugnux --model adopts the model's space group as a label where the data were merged in its enantiomorph, instead of reindexing the reflections - which swapped I(+) with I(-).
• rugnux --model warns, naming the atom, when the anomalous density at the model's atoms comes out inverted, which means the data and the model are in opposite hands.
• rugnux --model writes an anomalous difference map (<prefix>_anom.ccp4) when the merge kept the Bijvoet split, and names the ten model atoms it peaks highest on as ANOMALOUS_SITE_01.._10.
• MEAN_ATOM_DENSITY_SIGMA is read from the map by cubic rather than linear interpolation and comes out around a tenth higher; it is no longer comparable with the figure earlier versions printed.
• rugnux --model reads an mmCIF coordinate file as well as a PDB one, gzipped or not, taking the format from the file's content rather than its name.
• A model rugnux --model cannot use is reported as a WARNING: line in the results report instead of only in the log.
• The rugnux results report has a 10. MODEL VALIDATION section when --model was given; REPORT_VERSION is 4, WARNINGS moves to section 11 and no existing key changed.
• The rugnux results report records how the run was invoked, what it cost and what it ran on: COMMAND_LINE=, WALL_TIME= and GPU_COUNT= / GPU=.
• rugnux says which GPUs it can see before it starts processing.
• rugnux --export-unmerged also writes <prefix>_unmerged.mtz on a --no-merge run, and is ignored on a run with no output prefix instead of writing a file called _unmerged.mtz.
• /start asks the writer whether the run can be written before the detector is armed, so a run whose master file already exists, or whose output directory cannot be created, is refused up front with the writer's own message. This needs the TCP image stream or the built-in HDF5 writer; the ZeroMQ stream is unchanged.
• A calibration that fails goes to Error carrying the reason instead of Inactive, so /wait_till_done and /wait_until_running report it; a cancelled calibration still goes to Inactive.
• /wait_till_done answers 500 with the message when a collection ended in an error. A cancelled collection and a collection that only triggered a warning still answer 200.
• A pending start failure is discarded by /cancel and /deactivate, as it already was by /start and /initialize.
• /scan_result no longer reports the previous run's images after a collection that failed to start, or after /deactivate.
• The TCP image stream protocol version is 4. jfjoch_writer and jfjoch_broker have to be of the same release, as before.

* `rugnux --model` adopts the model's space group as a label where the data were merged in its enantiomorph, instead of reindexing the reflections - which swapped I(+) with I(-). * `rugnux --model` warns, naming the atom, when the anomalous density at the model's atoms comes out inverted, which means the data and the model are in opposite hands. * `rugnux --model` writes an anomalous difference map (`<prefix>_anom.ccp4`) when the merge kept the Bijvoet split, and names the ten model atoms it peaks highest on as `ANOMALOUS_SITE_01`..`_10`. * `MEAN_ATOM_DENSITY_SIGMA` is read from the map by cubic rather than linear interpolation and comes out around a tenth higher; it is no longer comparable with the figure earlier versions printed. * `rugnux --model` reads an mmCIF coordinate file as well as a PDB one, gzipped or not, taking the format from the file's content rather than its name. * A model `rugnux --model` cannot use is reported as a `WARNING:` line in the results report instead of only in the log. * The rugnux results report has a `10. MODEL VALIDATION` section when `--model` was given; `REPORT_VERSION` is 4, `WARNINGS` moves to section 11 and no existing key changed. * The rugnux results report records how the run was invoked, what it cost and what it ran on: `COMMAND_LINE=`, `WALL_TIME=` and `GPU_COUNT=` / `GPU=`. * rugnux says which GPUs it can see before it starts processing. * `rugnux --export-unmerged` also writes `<prefix>_unmerged.mtz` on a `--no-merge` run, and is ignored on a run with no output prefix instead of writing a file called `_unmerged.mtz`. * `/start` asks the writer whether the run can be written before the detector is armed, so a run whose master file already exists, or whose output directory cannot be created, is refused up front with the writer's own message. This needs the TCP image stream or the built-in HDF5 writer; the ZeroMQ stream is unchanged. * A calibration that fails goes to `Error` carrying the reason instead of `Inactive`, so `/wait_till_done` and `/wait_until_running` report it; a cancelled calibration still goes to `Inactive`. * `/wait_till_done` answers 500 with the message when a collection ended in an error. A cancelled collection and a collection that only triggered a warning still answer 200. * A pending start failure is discarded by `/cancel` and `/deactivate`, as it already was by `/start` and `/initialize`. * `/scan_result` no longer reports the previous run's images after a collection that failed to start, or after `/deactivate`. * The TCP image stream protocol version is 4. `jfjoch_writer` and `jfjoch_broker` have to be of the same release, as before.

leonarski_f added 22 commits 2026-08-27 22:10:28 +02:00

Broker: refuse a run the writer cannot write before arming the detector ... 41983ca589

A writer refusing to overwrite an existing file was reported badly, and the
earlier fix only covered half of it. The commit message of a6a715703 states the
assumption that broke: "SendStartMessage precedes every std::async in the
receiver constructor". That is true of JFJochReceiverFPGA, where the start
message goes out from the constructor body, and false of JFJochReceiverLite,
where it goes out from MeasurementThread - launched by std::async from the
constructor, which waits only on data_analysis_started. On the DECTRIS path the
writer's refusal therefore cannot reach services.Start(); it arrives from
services.Stop(), i.e. the second try-block of the state machine's
MeasurementThread, which never touched start_exception. /wait_till_done answered
200 for a run that never wrote anything.

Two changes, in front of the problem and behind it.

In front: a PREFLIGHT frame (TCP protocol version 3 -> 4), sent to every writer
before the receiver is built and before the detector is armed. It carries the
same CBOR start message a START does and asks one question: could this run be
written? The writer runs FileWriter::Preflight - the first three statements of
the FileWriter constructor, stopping short of the only one that creates
anything - and answers with an ACK. Shared code, so a pre-flight that passes
cannot be contradicted by the start behind it. A refusal is not fatal: nothing
was started, the connection stays usable, and above all no series is left armed
waiting for a trigger that will not come. Both ends already reject a frame of
another protocol version, so the bump costs no compatibility.

The check now covers the data files, not only the master. A stale
_data_000001.h5 used to sail through and fail when the file was renamed into
place at the end of the run, after the whole dataset had been collected. The
writer holding the master file checks for all of them - they share a directory
by construction, the master linking them by relative name - so the siblings do
not race each other statting files the others are creating.

Behind: /wait_till_done maps an Idle state to 200 or 500 by the severity of the
end-of-run message, so a collection that lost packets, or whose writer could not
write, is no longer indistinguishable from a good one to a script. A cancelled
collection is not affected - MeasurementThread checks the cancel before it looks
at the completeness of the data, and reports it with Info severity - and a
warning is still a 200. start_exception is untouched.

The PREFLIGHT payload is built from FillMessage alone, which is what keeps it
small: the per-pixel arrays are added by SendStartMessage and are not in it.
Measured on a JUNGFRAU 9M, 1.4 kB against 543 kB for the start message. It is
deliberately the whole of FillMessage rather than the fields the check reads
today - a hand-picked subset would answer for a different set of file names the
day the check learns to read one more field, and would do it silently.

Three defects found on the way:

TCPImagePuller republished a PREFLIGHT as a real start message, announcing to
downstream consumers a run that was not going to happen.

TCPStreamPusher::run_number was a plain uint64_t written by the control thread
and read by every ACK thread; Preflight adds a second writer to it.

StreamWriter::CollectImages reprocessed the last frame whenever WaitForImage
returned false - run is only tested at the top of the loop - so an abort wrote
the last image a second time. Pre-existing; the new branch made it visible.

Verified against a running broker on the local configuration: a colliding master
file and a colliding data file are both refused by /start, /wait_until_running
and /wait_till_done with the writer's own message, and the log shows the receiver
was never started for either; a cancelled collection still answers 200.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016WmryXe8ASbNi632sUMfsa

rugnux: write the unmerged MTZ on every run, not on request ... 472f7bcd47

<prefix>_unmerged.mtz is what another scaling program needs - aimless,
pointless, careless, iotbx.merging_statistics - and a run that did not ask for
it up front had to be repeated to get it. It is now written on every run that
has an output prefix, in --mode mx and --mode scale and with --no-merge,
alongside the merged files and replacing none of them. --no-export-unmerged
turns it off; --export-unmerged is still accepted and now does nothing.

The summed form is the default one, as before: each rocking event's partials
combined into one full, written at the batch its rocking curve is centred on.
--export-unmerged-partials still additionally writes the unsummed
<prefix>_unmerged_partials.mtz for a program that would rather sum them itself.

Both write sites gated on the export flag alone and would have written a file
called "_unmerged.mtz" when there was no output prefix. That could not happen
while the export was opt-in and a prefix was implied by asking for it; with the
export on by default it would fire on any prefix-less run. They now use the same
guard the merged writes use.

Verified on the rotation test dataset: written by default, by --mode scale and
under --no-merge, absent with --no-export-unmerged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016WmryXe8ASbNi632sUMfsa

Version 1.0.0-rc.165 ... ee2227e23c

Generated clients, Redoc, frontend, docs, FPGA and PCIe driver version strings
regenerated by update_version.sh.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016WmryXe8ASbNi632sUMfsa

rugnux: record in the report how the run was invoked, what it cost and what it ran on ... e4dce1d778

The results report said what the run determined but nothing about how it was
produced, so a report read next to a lost shell history could not be reproduced
or compared. Four keys in the header block, all new, none renamed - a consumer
that greps for what it needs is unaffected, so REPORT_VERSION does not move:

  COMMAND_LINE= rugnux -o myrun --model model.pdb dataset_master.h5
  WALL_TIME= 262.41
  GPU_COUNT= 4
  GPU= 4x NVIDIA A100-SXM4-80GB

The command line is argv as one shell-ready line; an argument that would not
survive being pasted back into a shell is single-quoted, so a file prefix with a
space comes back as the one argument it was. It replaces the raw argv echo at the
top of the run, which had no quoting at all.

WALL_TIME is the whole invocation, timed from the top of the CLI. It is
deliberately not result.total_time_s, which starts inside Rugnux::Run and so
counts neither opening the file nor setting up the analysis - and which --mode
scale never sets at all, having no ProcessResult of its own. It is printed on
stdout as well, next to the processing time it is slightly larger than.

The GPUs are the reason rugnux is fast, and until now nothing said whether any
were being used. get_gpu_names() reports them per device and get_gpu_description()
collapses repeats, so four identical cards read as one line rather than the same
name four times and a mixed machine keeps one group per model. Both have a
CPU-only implementation, so the JFJOCH_USE_CUDA=OFF build reports GPU_COUNT= 0
rather than failing to link.

The same line is printed at startup, before the run rather than after it: a
machine that turns out to have no GPU - a driver mismatch, a CUDA_VISIBLE_DEVICES
left over from another job - is worth knowing about while there is still time to
stop, not once the run has taken an order of magnitude longer than it should.
GPU_COUNT= 0 is written with no GPU= line beside it, because the absence is the
statement.

The header comment claiming timing is deliberately absent from the report is now
wrong and says so: rates and per-image costs stay on stdout, the total does not.

Verified on the rotation test dataset in both build configurations, with and
without CUDA_VISIBLE_DEVICES, in --mode mx and --mode scale.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016WmryXe8ASbNi632sUMfsa

rugnux: read mmCIF models with --model, and say when a model could not be used ... 18d3f55325

--model called gemmi::read_pdb unconditionally, so a coordinate file in mmCIF -
which is how the PDB serves coordinates by default - was refused outright:

  Model validation: cannot read model 6G8A.cif:
  Incorrect file format (perhaps it is cif not pdb?)

Worse than the refusal was what followed it. ValidateAgainstModel logged,
returned an empty result, and the run went on to finish with exit code 0, no
R-free, no maps and nothing in the report - indistinguishable from a run that was
never given --model at all. A script that passed a .cif, checked the exit code
and grepped for R-free simply got nothing back.

Both read sites now go through read_structure_gz with CoorFormat::Detect, so PDB,
mmCIF and mmJSON are all read, gzipped or not, and the format comes from the
file's content rather than from its name. Detect is passed explicitly: GEMMI
otherwise takes the extension and only falls back to the content when it does not
recognise one, and a model arrives named however whoever produced it named it.

That needed a part of GEMMI the vendored subset had trimmed away. mmread.hpp was
already here but its CIF parser was not, so this restores what upstream GEMMI
ships: read_cif/mmcif/json/mmread_gz, plus GEMMI's own copies of PEGTL (MIT,
Colin Hirsch and Daniel Frey; 155 headers, 672 kB) and sajson (MIT, Chad Austin;
one header). Both are listed in THIRD_PARTY_NOTICES.md with their own licence
texts, PEGTL's collected by COLLECT.sh and sajson's kept by hand, its terms being
a comment block rather than a file. json.cpp carries a one-line change: upstream
keeps sajson at the root of its repository, outside the include tree this subset
copies, so the include points at where the other bundled headers live.

ModelValidationResult gains failure_reason, and a model that was asked for and
could not be used now reaches the results report as

  WARNING: Model validation did not run: model bogus.pdb has no atoms or no unit cell

in --mode mx and --mode scale alike. The run still succeeds - a merge that is good
is not made bad by a model that was not - but it no longer does so quietly.

One log line was making a claim it could not support: the indexing-ambiguity
message said the ambiguity was "resolved against the supplied model" while being
printed before the model is read, so it announced a resolution that a failed model
never performed. It now says the model will be used; the reference branch, where
the work really has already happened, keeps the past tense.

Verified on the rotation test dataset with a deposited mmCIF, the same file
gzipped, an mmCIF under a .pdb name, a PDB, and an unreadable file.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016WmryXe8ASbNi632sUMfsa

rugnux: report what --model found, and attribute GEMMI's bundled headers ... bf4712a3d5

Two things left open by the previous commit.

A model that was validated left nothing in the results report. R-work, R-free,
the bulk-solvent parameters and the map filenames went to the log and nowhere
else, so a run given --model was, in the file anyone reads afterwards,
indistinguishable from one that was not - and the WARNING line added last commit
was the only trace of a model that failed.

Section 10. MODEL VALIDATION now carries it: MODEL_FILE, the space group the
model is in, R_WORK / R_FREE with their reflection counts, the bulk-solvent
k_sol / b_sol and overall scale, the mean 2Fo-Fc density at the atom centres, the
hand and indexing operators the written reflections were brought into the model's
frame with, and MAPS_PREFIX. A model that could not be used gets
MODEL_VALIDATION= NOT_PERFORMED and the reason instead, in the shape the merging
section already uses. A run that asked for no model has no section at all.

That moves WARNINGS from 10 to 11, which is what REPORT_VERSION is for: it is now
4. No existing key was renamed and no table column moved, so a consumer that greps
for what it needs is unaffected; one that keys on the section number of WARNINGS
is not, hence the bump.

The four third-party headers GEMMI has always bundled - fast_float, half,
pocketfft and tinydir - were never listed in THIRD_PARTY_NOTICES.md, riding
instead under GEMMI's single MPL-2.0 row. They are other authors' code under
other licences (Apache-2.0 OR MIT OR BSL-1.0, MIT, BSD-3-Clause, BSD-2-Clause),
and MPL-2.0 does not speak for them. Each now has its own row and its own licence
text, alongside the PEGTL and sajson entries added with the CIF parser, and a note
records why the six are listed separately from GEMMI. Their terms live in the
headers rather than in LICENSE files, so five of the six texts are kept by hand;
COLLECT.sh says so and copies the one - PEGTL - that ships a file.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016WmryXe8ASbNi632sUMfsa

rugnux: --model names the strongest anomalous scatterers, not just R-free ... 799381d027

Where the merge kept the Bijvoet split - which the default Friedel-averaged
merge does, so this needs no -A - --model now computes an anomalous difference
Fourier, coefficients F(+)-F(-) on the model phase turned back by 90 degrees
over the acentric reflections, writes it as <prefix>_anom.ccp4, and reads it at
the model's own atom centres. The ten highest are logged and land in the results
report as ANOMALOUS_SITE_01..10, each named by the atom, residue and chain it
sits on, with its height in map sigma. Reading at the sites rather than
searching the map for blobs is what ANODE does, and it says what carries the
signal instead of leaving coordinates to look up; a scatterer the model does not
contain is by construction absent from the list, which is what the map file is
for.

The list is always ten long, so its information is in the heights. On an
S-SAD dataset at 5 keV the six cysteine sulfurs take ranks 1-6 (10.1 down to
4.2 sigma) and rank 7 falls to 2.2; the same crystal at 13 keV, where sulfur has
no anomalous signal to speak of, tops out at 2.7 sigma with a water and a main
chain carbon inside the top five. So the same list reads as a signal / no-signal
gate, not only as a ranking.

The reading is cubic. Grid::interpolate_value defaults to trilinear, and on the
d_min/3 map grid that under-reads a peak this sharp by up to a quarter, unevenly
enough to reorder the sites; cubic on the same grid is within about 2% of a
sample-rate-10 one. The map r.m.s. is unchanged by the sample rate, so the
normalisation was never at fault. The 2Fo-Fc reading behind
MEAN_ATOM_DENSITY_SIGMA is deliberately left on the default so a number already
in existing reports does not move.

An anomalous merge keeps each Bijvoet mate as a row of its own and both rows
carry the same F(+)/F(-) pair, attached to the + index of the Friedel ASU of the
frame the merge was made in, so the differences are read from the merged
reflections as they came in - carried into the model's frame here, on the +
rows only. Taking the - rows as well would give one reflection both signs of its
difference and let the last row written decide.

write_ccp4 splits into an FFT and a write so the anomalous map can be both
written and sampled from one transform; the 2Fo-Fc atom-centre score, which used
to repeat that FFT, now reuses the grid.

Cross-checked against ANODE run on this output: identical identification - its
three peaks are the three disulfides, each maximum 0.64-0.70 A off a sulfur we
name - with our heights about 1.35x lower. Sharpening, grid and sigma
normalisation are excluded; the residual is SHELXC's FA preparation, which fed
ANODE 2166 reflections against our 2534.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014zTy4Bpi4pPHw4bybf7q2R

rugnux: read the 2Fo-Fc map at the atom centres cubically too ... 347e76fd87

Grid::interpolate_value defaults to trilinear, and on the d_min/3 map grid that
under-reads a peak by around a tenth. The anomalous site list already asks for
cubic; MEAN_ATOM_DENSITY_SIGMA was left on the default so a number already in
existing reports would not move, but the same argument that makes cubic right
for one reading makes it right for the other, and the two should not disagree
about how a map is sampled. One named constant now carries the choice and the
reason for both.

Measured on two rotation datasets: 1.96 -> 2.20 and 2.69 -> 2.89 sigma. The
maps, the R-factors and every other reported number are unchanged - only this
reading of the map moves, so it no longer compares with what earlier versions
printed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014zTy4Bpi4pPHw4bybf7q2R

rugnux: --model takes the model's enantiomorph as a label, and stops flipping I(+)/I(-) ... 1d26e797ad

Where the data were merged in the enantiomorph of the model's space group,
--model reindexed the observed reflections by change_of_hand_op(). That was
wrong, and the anomalous site list added yesterday is what showed it.

The two groups of an enantiomorphic pair differ only in the translations of
their operations. Their rotations are identical, so they transform hkl
identically, share a reciprocal ASU, and split into Bijvoet hands identically -
to_asu_sign returns the same answer for either, verified over 200k random
indices for all eight enantiomorphic families. The label therefore carries no
handedness at all, and there was never anything about it to undo. What does
carry the hand is the indexing the data already have, from the diffraction
geometry, and with it the sign of every anomalous difference. change_of_hand_op
is the inversion, so reindexing by it swapped I(+) with I(-): it did not correct
the hand, it flipped it, on the strength of a label the space-group search
itself reports as undetermined.

So the model's group is now adopted as a label and no reflection moves. On a
tetragonal reference dataset the site list goes from ten unrelated atoms at
around 3 sigma to the two ordered halides at 15.6 and 10.3 sigma followed by the
methionine and every cysteine sulfur at 6.6 to 7.3 - the correct map was there
all along and the reindex was inverting it.

Whether the model and the data agree about the hand is a real question, and now
an answerable one: a map whose deepest hole at an atom is below -5 sigma and
deeper than its highest peak says they disagree, and the run warns and names
that atom. It is deliberately not repaired by reindexing, which would make the
two agree by construction and destroy the evidence for which of them is wrong.
Verified by mirroring a model: the warning fires naming the atom at -10.1 sigma,
where the unmirrored model reads +10.1. R-free is identical to four decimals
across that mirror, which is exactly why it could never have arbitrated this.

MODEL_HAND_OPERATOR becomes MODEL_ENANTIOMORPH_ADOPTED, there being no operator
to report. REPORT_VERSION stays 4: both that version and the MODEL VALIDATION
section it describes were added in this same unreleased cycle, so no report
carrying the old key has ever left it and there is nothing for a consumer to
have depended on. The unmerged export follows only the indexing operator now,
the enantiomorph being a label.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014zTy4Bpi4pPHw4bybf7q2R

Reindex into the ASU: follow the change of hand, not the hand landed on ... edc042aadf

ReindexMergedIntoAsu swapped I(+)/I(-) whenever the reindexed index came out on
the minus side of its Friedel pair. That is right only for a row that started on
the plus side, which is every row when merge_friedel is set - and none of the
problem cases.

With the mates kept apart, the merge stores the plus mate at +hkl_asu and the
minus mate at -hkl_asu (Merge.cpp), while RotationScaleMerge attaches I_plus /
I_minus in the plus convention on both rows alike, keying the lookup at
plus=true. An alternative-indexing operator is rotation-type, so op(-x) == -op(x)
and the two mates always land on opposite sides: testing key.plus alone therefore
swaps exactly one row of every pair, whichever way the operator went. The pair
ends up self-contradictory, and the mmCIF's pdbx_I_plus / pdbx_I_minus and the
F(+)/F(-) columns are swapped on half the rows.

Reached by "rugnux -A --model model.pdb" - -A clears MergeFriedel - on a crystal
whose indexing ambiguity the model resolves, which is the one path that both
keeps the mates apart and reindexes.

The swap now fires on was_plus != key.plus. With merge_friedel every row is
stored at the ASU representative, so was_plus is always true and this reduces to
the previous test; the existing change-of-hand case is unchanged.

The test that covered the merge_friedel=false path checked only I, so it passed
either way. The new one builds both mates of a pair and asserts they still agree
about which intensity is which afterwards - an invariant that holds however the
operator moves the hand, so it fails on the old code for either direction.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016L1qig74oYQzfUJJZbbxFh

rugnux: the unmerged MTZ is written on request again, not on every run ... 685c562174

0fed51f76 made <prefix>_unmerged.mtz default-on so a run that turned out to need
it would not have to be repeated. It costs more than that is worth: measured on a
dense rotation dataset, it adds 2.6 s to a 7.0 s --mode scale run (+38%) and
writes 272 MB - larger than the merged .mtz, .cif and .hkl put together, and paid
by every run whether or not anything will read it.

--export-unmerged asks for it again. --no-export-unmerged stays accepted and now
does nothing, the mirror of what --export-unmerged did while the default was on.

The two fixes that came with the default-on change are kept: the file is written
in --mode mx and --mode scale and with --no-merge, and both write sites are
guarded on the output prefix, so a prefix-less run no longer writes a file called
"_unmerged.mtz".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016L1qig74oYQzfUJJZbbxFh

Pre-flight: check the master file, not every data file of the run ... 95108276d1

5f838bdb2 widened CheckOutputFilesAvailable from the master file to every data
file the run will write, so a stale _data_000001.h5 would be caught up front
rather than at the rename. The enumeration is unbounded: it is one
std::filesystem::exists per planned file, so a long run with a small
images_per_file - it can be set to 1 - is thousands of lookups, and they run
inside the 5 s PREFLIGHT ACK budget that BUSY heartbeats do not extend. On a
shared filesystem that turns a writable output into an "ACK timeout", which is
both wrong and unreadable. It also called HDF5Metadata::DataFileName for every
planned file, and that throws past file 999999 (9999 on SwissFEL), refusing a run
up front on a file number the acquisition would never reach.

Back to the master file alone, which is what the constructor checked before. A
data file in the way is caught per-writer at finalize again, as it was.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016L1qig74oYQzfUJJZbbxFh

Broker: a failed calibration is an Error, and ending a run clears what it left ... fdfc9b1f90

Three things a caller could not see, all in the same lifecycle.

A calibration that FAILS now goes to Error rather than Inactive. The wait
endpoints turn Error into a 500 carrying the message, while Inactive is a
bodiless 502 - so the reason a pedestal or mask sequence failed was thrown away
on the way out. Inactive is also where a deliberate Deactivate() leaves the
broker, which made a failure indistinguishable from a detector someone had
powered down. Initialize() clears Error, so recovery is what it was. The four
CANCELLATION paths keep Inactive: a cancel is not a failure, and c29dd67b5
separated the two deliberately.

Cancel() and Deactivate() clear start_exception. JFJochStateMachine.h says every
entry point that begins new work clears it; these two end work instead, and were
missed. A failed /start followed by /deactivate left every later wait call
rethrowing a failure from a run on a detector that is no longer powered, until
some later /start or /initialize happened to clear it.

Start() and Deactivate() clear scan_result. It is only ever assigned at the end
of a successful measurement, so a run that failed to start left /scan_result
answering with the PREVIOUS run's images, under the new run number and with
nothing to mark them as not its own - the same stale-read Initialize() has always
cleared. Deactivate() clears it for the same reason it clears the rest: the
result describes a measurement on a detector being turned off.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016L1qig74oYQzfUJJZbbxFh

Changelog: one line per entry for rc.164 and rc.165 ... 6406427be4

Both sections had drifted into paragraphs carrying the rationale, the
investigation and what the change was measured on - all of which the commit
messages already hold, and which docs/CHANGELOG.md is explicitly not for. rc.162
is the shape they should have been in.

Rewritten to one line each, saying what changed. No entry is dropped except the
pre-flight data-file check, which is no longer the behaviour, and the entries for
this round's fixes are added.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016L1qig74oYQzfUJJZbbxFh

Pre-flight: always answer, and record the invariant the check rests on ... 9d23c2a7ee

ProcessPreflight caught JFJochException only. The pusher is blocked on this ACK
and reads its absence as a dead writer, so anything else escaping to Run()'s
generic handler cost the caller the real reason and handed it a five-second
timeout instead - for a check whose entire purpose is to report the problem
before the detector is armed. std::filesystem::exists is the concrete leak: it
throws filesystem_error, not JFJochException, when the output path cannot be
walked at all (a directory component with no search permission, a symlink loop),
which is exactly the misconfiguration worth catching early. Now catches
std::exception, as JFJochReceiverService::Start does.

Also corrects the comment above the write_master_file assignment in
TCPStreamPusher::Preflight, which claimed index 0 had to fall on the same
connection here as in StartDataCollection. It does not, and cannot be made to:
Preflight filters the pool on connected && !broken, while START re-derives its
index 0 after DetachDeadConnections, which additionally drops anything failing
IsConnectionAlive - so a writer lost in between shifts it. That is harmless,
because every writer of a run works on the same filesystem and so answers the
same question about the same files. The comment now says that instead.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016L1qig74oYQzfUJJZbbxFh

Changelog: restore rc.164 and rc.163, rewrite only rc.165 ... fb04a3977e

f9444278d rewrote the rc.164 section as well, and dropped rc.163 outright. Both
are released and published; a changelog entry that has gone out does not get
rewritten afterwards, whatever its shape.

The file is rebuilt from the published one with only the rc.165 section replaced,
so rc.164, rc.163 and everything older are byte-identical again. rc.165 keeps the
one-line-per-entry form - it is still unreleased, and that is the form
docs/CHANGELOG.md asks for.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016L1qig74oYQzfUJJZbbxFh

rugnux pre-scan: measure the width on settled geometry, and skip the unread projection ... e26083f039

Two things the pre-scan did regardless of what was asked of it.

The spot width is read off the geometry as it stands. Candidates are excluded
within MIN_BEAM_DISTANCE_PX of the beam centre and stratified into resolution
bands about it, so with --estimate-beam-center the width was measured against the
centre the pass was in the middle of correcting - a placeholder header centre
both selects a different set of spots and labels them at the wrong resolution -
and spot_width_measured_ then stopped the corrected pass from ever revisiting it.
The width is now left to the second pass where one follows, which starts from the
corrected centre; a run with no second pass measures it as before, a provisional
radius being better than none.

ShadowFinder::AddImage projects the whole image and ran for every frame of the
shadow sample whether or not a shadow was wanted - with --detect-beam-stop=off,
or on the second pass where the mask already holds one, that is the pass's
largest per-frame cost paid for a result nobody reads. Now gated on want_shadow.
The frame set is unchanged, so the mask is still built from the frames it was
validated on.

Gating it exposed the read-failure guard below, which asked the shadow finder how
many frames it had seen. With the finder no longer fed, a pass that read every
frame for the beam centre alone would have reported "no image could be read" and
returned before placing it. Frames read are now counted directly.

Inert on the shipped defaults, as intended: --estimate-beam-center is off by
default, so the width deferral cannot fire, and the projection is wanted on the
first pass. Verified by building the parent commit and this one and running eight
crystals of the rotation battery through both at -N 10: every column identical -
space group, unique reflections, observations, completeness, R_meas, CC1/2,
<I/sigma> - on all eight. Warm-cache wall clock 1m03s against 1m02s, i.e. a wash;
the saving only falls on passes that were discarding the result anyway.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016L1qig74oYQzfUJJZbbxFh

Anisotropy: do not report a shape the fit never produced, and read every axis ... f706b2d8d2

FitShape set out.shells before running the regressions and returned early if
either weighted least-squares call failed, leaving NaN coefficients with flat and
convex both false. AnalyzeAnisotropy tested exactly those two, so it read the
failure as Linear and marked it stable: the report and _reflns.jfjoch_aniso_shape
claimed LINEAR over a shell count that suggested it meant something, with nan
printed beside it. ShapeFit now carries an explicit ok, set only once every
regression has succeeded, and result.shape_shells reports the shells the
signature was actually fitted over - none, where it was not - so the renderer and
the rebinning caution skip it and the verdict stays Undetermined.

The directional-limit line tested the first and third principal axes for a finite
d_min but not the second. ConeLimits fills each independently, so a crystal whose
outer two directions fall in cones too sparse to measure while the middle one is
well covered suppressed the whole line, the d_min spread with it. All three are
tested now.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016L1qig74oYQzfUJJZbbxFh

rugnux --model: say why validation did not run when the data carry no resolution ... 06a8a9d143

The d_min <= 0 return was the one failure path that left failure_reason empty.
The report's MODEL_VALIDATION= NOT_PERFORMED line and the WARNING that goes with
it are both gated on that string, so this case printed a blank reason and no
warning - the "indistinguishable from a run that was never given --model" outcome
the field was added to prevent. All four failure returns now set it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016L1qig74oYQzfUJJZbbxFh

rugnux: use the project's PI in the anomalous map phase, not M_PI ... 02f3759ca2

M_PI is not standard C++ and MSVC does not define it without _USE_MATH_DEFINES,
so the Windows rugnux build failed on the anomalous-map phase shift added in
b0234a1b4:

  ModelValidation.cpp(350): error C2065: 'M_PI': undeclared identifier

common/JFJochMath.h carries a plain constexpr PI for exactly this reason - its
header comment names MSVC - and BraggIntegrationEngine.cpp already uses it that
way. Same idiom here, same include comment.

The digits are those of glibc's M_PI, so the phase, the map and the site heights
are bit-identical; this is a portability fix and nothing else. Verified that no
other M_PI is left anywhere in the viewer-reachable subset, and that nothing in
the files this branch adds or changes there reaches for a GCC builtin, a POSIX
header or __attribute__.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016L1qig74oYQzfUJJZbbxFh

Writer: a START that fails for a reason of someone else's type still answers ... b9d77e63dd

ProcessStartMessage caught JFJochException only, the same narrow catch
ProcessPreflight had. Everything that makes a failed START reportable sits inside
that handler - the Error state, err, and the fatal ACK - so an exception of any
other type skipped all three and left the broker blocked on an ACK that never
came, with the detector about to be armed. It reads that silence as a dead writer
and reports a timeout instead of the reason, and the writer is left out of the
Error state it should have entered.

Not every thrower on this path is ours: std::filesystem::exists throws
filesystem_error when the output path cannot be walked at all (a directory
component with no search permission, a symlink loop), and
SetupFinalizedFileSocket throws ZeroMQ's own type. Now catches std::exception.
Logger::ErrorException already takes std::exception, so the handler body is
unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016L1qig74oYQzfUJJZbbxFh

Pre-flight: catch a data-file collision too, not just the master file ... ad25d5bff0

The pre-flight only checked the master file name, so a data file already
sitting in the output's way was refused only when the run had been written
in full and the first colliding data file failed to rename into place at
the end of it. Checking every candidate name individually would cost one
round trip per file on a network filesystem, so a single directory listing
is taken once and compared against the run's expected file names in memory.

leonarski_f force-pushed rc165 from 1ab68d3871 to ad25d5bff0 2026-08-27 22:10:28 +02:00 Compare

leonarski_f merged commit 511be0c366 into main 2026-08-27 22:16:54 +02:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

No items

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

appleb_m (appleb_m) buntschu_d (buntschu_d) duan_j (duan_j) glettig_w (glettig_w) gotthard_g (gotthard_g) leonarski_f (leonarski_f) mx-gitea perl_d (perl_d) takaba_k (takaba_k)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: mx/Jungfraujoch#75