`_add_variances` mutates its input and silently adds nothing for binned data

[SimonHeybrock]

_add_variances (packages/essreduce/src/ess/reduce/nexus/workflow.py:750), used by assemble_monitor_data and assemble_detector_data:

def _add_variances(da: sc.DataArray) -> sc.DataArray:
    out = da.copy(deep=False)
    if out.bins is not None:
        content = out.bins.constituents['data']
        content.data = _assign_values_as_variances(content.data)
    elif out.variances is None:
        out.data = _assign_values_as_variances(out.data)
    return out


def _assign_values_as_variances(var: sc.Variable) -> sc.Variable:
    try:
        var.variances = var.values
    except sc.VariancesError:
        var = var.to(dtype=sc.DType.float64)
        var.variances = var.values
    return var

Three observable defects, one root cause.

It mutates its input. out = da.copy(deep=False) reads as protection for the caller's data, and the out.data = ... idiom is the right one — rebinding .data on a shallow-copied DataArray genuinely does not affect the original. But _assign_values_as_variances sets .variances on the Variable in place and returns that same object, so the input has already been modified by the time the rebind runs. The rebind is a no-op and the shallow copy achieves nothing.

For binned data it silently produces no variances whenever the in-place write is refused. content = out.bins.constituents['data'] yields a DataArray whose .data rebind does not write back into the binned variable, so on that branch only the in-place mutation has any effect. scipp refuses to add variances through a sliced view (VariancesError: Cannot add variances via sliced view of Variable), and the .to(dtype=...) fallback copy is then discarded. Result: data without variances, no error.

Same for integer event buffers, for the same reason. The non-binned branch handles integers correctly, so the two branches disagree.

Reproducer (scipp 26.3.1, essreduce 26.6.3):

import scipp as sc
from ess.reduce.nexus.workflow import _add_variances

def binned(buffer: sc.Variable) -> sc.DataArray:
    events = sc.DataArray(buffer, coords={'tof': sc.arange('event', buffer.sizes['event'])})
    begin = sc.array(dims=['pulse'], values=[0], unit=None, dtype='int64')
    return sc.DataArray(sc.bins(begin=begin, dim='event', data=events))

# (a) mutates its input
owned = sc.ones(sizes={'event': 4}, dtype='float64', unit='counts')
_add_variances(binned(owned))
print('(a) input buffer gained variances:', owned.variances is not None)

# (b) silent no-op when the event buffer is a sliced view
sliced = sc.ones(sizes={'event': 8}, dtype='float64', unit='counts')['event', :4]
out = _add_variances(binned(sliced))
print('(b) result has variances:', out.bins.constituents['data'].variances is not None)

# (c) silent no-op for integer event buffers
ints = sc.ones(sizes={'event': 4}, dtype='int64', unit='counts')
out = _add_variances(binned(ints))
print('(c) result has variances:', out.bins.constituents['data'].variances is not None,
      '| dtype:', out.bins.constituents['data'].dtype)

# non-binned integer input, for contrast: this branch is correct
da = sc.DataArray(sc.ones(sizes={'x': 4}, dtype='int64', unit='counts'))
out = _add_variances(da)
print('    non-binned int: result has variances:', out.variances is not None,
      '| input untouched:', da.variances is None)

(a) input buffer gained variances: True
(b) result has variances: False
(c) result has variances: False | dtype: int64
    non-binned int: result has variances: True | input untouched: True

Minor, and part of the same picture: unlike the non-binned branch, the binned branch has no variances is None guard, so variances already present are overwritten rather than left alone.

Presumably none of this has surfaced because file-based use does not trigger it. scippnexus creates event weights as freshly owned float32 ones (nxevent_data.py, "Weights are not stored in NeXus, so use 1s"), so the in-place write succeeds and the buffer it mutates is one nobody else holds. The defects need a reused or shared event buffer to become visible.

How it shows up for us

In esslivedata the event buffers reaching assemble_detector_data/assemble_monitor_data are reused, zero-copy slices of a preprocessor-owned buffer, handed by reference to jobs that run concurrently in a thread pool. Case (b) is our steady state, so reduction results silently lose their uncertainties. Case (a) fires whenever a batch's event count happens to equal the buffer capacity, and is then an unsynchronized structural mutation of a Variable that other threads are reading.

Implications of possible fixes

Not a proposal, just what we measured while working out our own options, since the choice has costs that fall on all users.

Making it non-mutating and writing back correctly. For example:

def _values_as_variances(var: sc.Variable) -> sc.Variable:
    var = var.to(dtype=sc.DType.float64, copy=True)
    var.variances = var.values
    return var


def _add_variances(da: sc.DataArray) -> sc.DataArray:
    if da.bins is not None:
        constituents = da.bins.constituents
        content = constituents['data']
        if content.variances is None:
            constituents['data'] = content.assign(_values_as_variances(content.data))
            da = da.assign(sc.bins(**constituents))
    elif da.variances is None:
        da = da.assign(_values_as_variances(da.data))
    return da

This gives variances in all four cases above, leaves inputs untouched, and promotes integer buffers to float64. Where the current code succeeds in place it allocates one new array (the variances); this allocates two, a copy of the values plus the variances.

Measured on binned events carrying event_time_offset and event_id (float64 weights, int32 coords, so 16 B/event), one timed call per freshly built input so that no call sees variances left behind by a previous one:

events	event data	current, owning	above, owning	current, sliced	above, sliced
20M	0.3 GB	23 ms	35 ms	47 ms	35 ms
100M	1.6 GB	85 ms	120 ms	190 ms	124 ms
400M	6.4 GB	327 ms	461 ms	698 ms	462 ms

Linear in event count: 0.82 ns/event today against 1.15 ns/event, so +41% on the path that currently succeeds in place. Where the event buffer is a view it is faster, 1.75 down to 1.16 ns/event, because the current code copies via the exception fallback and then discards the result.

Peak RSS attributable to the call:

events	event data	current	above
100M	1.6 GB	+0.78 GB	+1.56 GB
400M	6.4 GB	+3.13 GB	+6.25 GB

For file-based reduction over multi-GB event data the extra time is small next to reading the file, but the transient allocation doubles: on a 6.4 GB event file, peak RSS for this call goes from 9.5 GB to 12.6 GB.

Adding the missing variances is None guard to the binned branch. Fixes the unconditional overwrite and makes the function free for input that already carries variances. On its own it fixes neither the mutation nor the silent loss.

Creating the event weights with variances where the ones-array is built, so that _add_variances has nothing to do. Combined with the guard above this costs nothing — no copy, no second allocation — and sidesteps both the mutation and the loss for that producer.

This last one is what we would favour for our own path, and it is what #677 is about. Worth being explicit that it is not a fix for the defects above: it keeps one producer out of the buggy path, while _add_variances remains as-is for integer event buffers and for caller-supplied data without variances. It also depends on the guard — without it the binned branch re-attaches unconditionally, so pre-attached variances are overwritten in place and the write to shared data remains.