Cost of unconditionally adding variances in `assemble_*_data`
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 1
- Forks
- 3
- Avg merge
- 2d 11h
- Merged PRs (30d)
- 17
Description
assemble_detector_data/assemble_monitor_data always attach Poisson variances to event weights. For consumers that discard uncertainties before histogramming, that work cannot be avoided today.
esslivedata's detector views project raw events to screen coordinates and then histogram them. The projection provider's first act is to throw the variances away, because per-event uncertainties are of no use there and roughly double the cost of everything downstream. We have carried a TODO about it:
# TODO Can we modify the provider in ess.reduce to not add variances in the first
# place (optionally)? This is wasteful here, if variances are needed they can be
# added after histogramming.
detector = sc.values(detector)
Measured on binned events carrying event_time_offset and event_id (float64 weights, int32 coords, so 16 B/event), per call:
| events | event data | _add_variances, sliced buffer |
sc.values |
|---|---|---|---|
| 20M | 0.3 GB | 47 ms | 87 ms |
| 100M | 1.6 GB | 190 ms | 327 ms |
| 400M | 6.4 GB | 698 ms | 1265 ms |
Both linear, 1.75 and 3.16 ns/event. On our live batches that is single-digit ms per batch, though it is per batch and per job with several detector-view jobs running concurrently against one source; run file-based over 6.4 GB of events it is around two seconds per call. Peak RSS rises by GB at those sizes.
Note the first column currently buys nothing at all on our inputs: on a sliced event buffer _add_variances copies, allocates, frees and returns data with no variances — see #676. The second column then copies the whole event table to strip variances that were never attached. So the present cost is not the price of uncertainties, it is the price of failing to attach them.
For unit weights, attaching variances after histogramming is exactly equivalent and operates on bins rather than events:
a = sc.DataArray(sc.ones(sizes={'event': n}, unit='counts', with_variances=True),
coords={'toa': toa}).hist(toa=edges)
b = sc.DataArray(sc.ones(sizes={'event': n}, unit='counts'),
coords={'toa': toa}).hist(toa=edges)
b.variances = b.values
assert sc.identical(a, b)
That equivalence holds only while weights are unit — a workflow applying per-event weights before histogramming needs the sum of squared weights, not the count.
What we might favour
Create the event weights with variances where the ones-array is built, rather than attaching them afterwards, with a flag to turn that off for pipelines that do not want event-level variances. Producing them costs nothing at the point of creation, so consumers that want uncertainties pay no copy, and consumers that discard them — raw detector views here — pay nothing at all and can attach Poisson variances after histogramming as above.
Taken far enough that would mean dropping _add_variances from the assemble providers altogether, on the understanding that variances are established earlier in the graph. Four things that shape whether that is practical:
- For file-based loading the weights are not created in essreduce but in scippnexus (
nxevent_data.py, "Weights are not stored in NeXus, so use 1s",float32). So "earlier" is either a scippnexus change or a new provider in essreduce adjacent toload_nexus_data. The latter keeps the change here, and such a provider owns the array it just created, so attaching in place is legitimate and costs nothing. - It is two producer paths, not one.
_add_variances's non-binned branch is also what promotes dense integer counts to float64 with Poisson variances, which is how histogram monitors get their uncertainties. That case needs its own answer. - Assuming variances were established earlier is an implicit contract that fails quietly if it is not met: results come out with bare values and no error, which is symptom (b) of #676 in a new place. A check that raises when variances are wanted but absent would fail loudly instead.
- It relies on the missing
variances is Noneguard in the binned branch; without it the weights are re-attached in place regardless of what the producer did.
None of this fixes _add_variances itself, which stays as-is for integer event buffers and for caller-supplied data.
For contrast, resolving #676 by making _add_variances non-mutating instead makes the copy unconditional, which raises the cost of the always-on behaviour rather than lowering it.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the assemble_detector_data and assemble_monitor_data providers, _add_variances, and the TODO in the detector projection path. Read scippnexus/nxevent_data.py and the code adjacent to load_nexus_data to compare the file-based and non-binned producer paths. Done means the design handles optional event variances, preserves required monitor uncertainties, and avoids unnecessary copying for consumers that discard them.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- data, performance
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Needs clarification
- Newbie friendliness
- 35/100