From 73b649d8d880d4677d385157a3c68d02bb73959b Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Mon, 9 Nov 2026 10:52:31 +0000 Subject: [PATCH 01/16] Replace pixel mask map/reduce with a list of filenames Workflows now take PixelMaskFilenames and read all mask files in one provider, instead of mapping over the files with sciline's map/reduce. This is the change that scipp/ess#746 lists for esssans and essdiffraction. The result does not depend on the number of files, so an empty list needs no special case. The reader and the conversion to boolean masks move to ess.reduce.masking, so that esssans and essdiffraction share them. The reader selects the format by file suffix (ISIS XML or scipp HDF5). Masks are named after the file without its directory, since directories differ between hosts. Two files with the same name raise an error, because one mask would otherwise replace the other. Breaking: with_pixel_mask_filenames, PixelMaskFilename and MaskedDetectorIDs are removed. esssans and essdiffraction need a release of essreduce that contains ess.reduce.masking. Co-Authored-By: Claude Opus 5.5 --- .../docs/api-reference/index.md | 1 - .../beer/beer_powder_mcstas_analytical.ipynb | 2 +- .../common/vanadium_processing.ipynb | 3 +- .../dream-advanced-powder-reduction.ipynb | 8 +- .../dream/dream-powder-reduction.ipynb | 2 +- .../POWGEN_data_reduction.ipynb | 3 +- .../src/ess/dream/parameters.py | 6 +- .../essdiffraction/src/ess/dream/workflows.py | 5 - .../essdiffraction/src/ess/powder/__init__.py | 2 - .../essdiffraction/src/ess/powder/masking.py | 58 +-------- .../essdiffraction/src/ess/powder/types.py | 8 +- .../essdiffraction/src/ess/powder/workflow.py | 6 +- .../tests/beer/mcstas_reduction_test.py | 4 +- .../tests/dream/geant4_reduction_test.py | 26 +--- .../tests/powder/masking_test.py | 31 +++++ .../snspowder/powgen/powgen_reduction_test.py | 13 +- .../tools/docs/beer-thumbnails.ipynb | 4 +- .../tools/docs/dream-thumbnails.ipynb | 2 +- .../essreduce/docs/api-reference/index.md | 1 + .../docs/user-guide/polarization/zoom.ipynb | 2 +- packages/essreduce/src/ess/reduce/__init__.py | 1 + packages/essreduce/src/ess/reduce/masking.py | 122 ++++++++++++++++++ packages/essreduce/tests/masking_test.py | 84 ++++++++++++ packages/esssans/docs/api-reference/index.md | 1 - .../esssans/docs/user-guide/isis/zoom.ipynb | 2 +- .../user-guide/loki/loki-direct-beam.ipynb | 4 +- .../docs/user-guide/loki/loki-iofq.ipynb | 4 +- packages/esssans/src/ess/isissans/data.py | 24 ++-- packages/esssans/src/ess/isissans/sans2d.py | 5 +- packages/esssans/src/ess/isissans/zoom.py | 2 - packages/esssans/src/ess/loki/data.py | 8 +- .../esssans/src/ess/loki/larmor_workflow.py | 6 +- packages/esssans/src/ess/sans/__init__.py | 2 - packages/esssans/src/ess/sans/io.py | 42 ------ packages/esssans/src/ess/sans/masking.py | 26 ++-- packages/esssans/src/ess/sans/parameters.py | 6 +- packages/esssans/src/ess/sans/types.py | 7 +- packages/esssans/src/ess/sans/workflow.py | 29 ----- .../tests/isissans/zoom_reduction_test.py | 13 +- packages/esssans/tests/loki/iofq_test.py | 17 +-- packages/esssans/tests/loki/workflow_test.py | 4 +- 41 files changed, 324 insertions(+), 272 deletions(-) create mode 100644 packages/essdiffraction/tests/powder/masking_test.py create mode 100644 packages/essreduce/src/ess/reduce/masking.py create mode 100644 packages/essreduce/tests/masking_test.py diff --git a/packages/essdiffraction/docs/api-reference/index.md b/packages/essdiffraction/docs/api-reference/index.md index cf04faf3d..f06acda05 100644 --- a/packages/essdiffraction/docs/api-reference/index.md +++ b/packages/essdiffraction/docs/api-reference/index.md @@ -36,7 +36,6 @@ .. autosummary:: :toctree: ../generated/functions - with_pixel_mask_filenames ``` ### Submodules diff --git a/packages/essdiffraction/docs/user-guide/beer/beer_powder_mcstas_analytical.ipynb b/packages/essdiffraction/docs/user-guide/beer/beer_powder_mcstas_analytical.ipynb index c9d64ea9f..50ab9cbd3 100644 --- a/packages/essdiffraction/docs/user-guide/beer/beer_powder_mcstas_analytical.ipynb +++ b/packages/essdiffraction/docs/user-guide/beer/beer_powder_mcstas_analytical.ipynb @@ -107,7 +107,7 @@ "workflow[KeepEvents[VanadiumRun]] = KeepEvents(True)\n", "workflow[KeepEvents[EmptyCanRun]] = KeepEvents(True)\n", "\n", - "workflow[MaskedDetectorIDs] = MaskedDetectorIDs({})\n", + "workflow[LoadedPixelMasks] = LoadedPixelMasks({})\n", "workflow[UncertaintyBroadcastMode] = UncertaintyBroadcastMode.drop" ] }, diff --git a/packages/essdiffraction/docs/user-guide/common/vanadium_processing.ipynb b/packages/essdiffraction/docs/user-guide/common/vanadium_processing.ipynb index 4b8dc130a..f95b9dd93 100644 --- a/packages/essdiffraction/docs/user-guide/common/vanadium_processing.ipynb +++ b/packages/essdiffraction/docs/user-guide/common/vanadium_processing.ipynb @@ -40,7 +40,6 @@ "import scippneutron as scn\n", "import scippneutron.peaks\n", "\n", - "from ess import powder\n", "from ess.snspowder import powgen\n", "from ess.powder.types import *" ] @@ -78,7 +77,7 @@ "workflow[TwoThetaMask] = None\n", "workflow[WavelengthMask] = None\n", "\n", - "workflow = powder.with_pixel_mask_filenames(workflow, [])" + "workflow[PixelMaskFilenames] = []" ] }, { diff --git a/packages/essdiffraction/docs/user-guide/dream/dream-advanced-powder-reduction.ipynb b/packages/essdiffraction/docs/user-guide/dream/dream-advanced-powder-reduction.ipynb index f02589fe6..9458db9d3 100644 --- a/packages/essdiffraction/docs/user-guide/dream/dream-advanced-powder-reduction.ipynb +++ b/packages/essdiffraction/docs/user-guide/dream/dream-advanced-powder-reduction.ipynb @@ -73,7 +73,7 @@ "workflow[DspacingBins] = sc.linspace(\"dspacing\", 0.3, 2.3434, 201, unit=\"angstrom\")\n", "\n", "# Do not mask any pixels / voxels:\n", - "workflow = powder.with_pixel_mask_filenames(workflow, [])" + "workflow[PixelMaskFilenames] = []" ] }, { @@ -194,7 +194,7 @@ "workflow[DspacingBins] = sc.linspace(\"dspacing\", 0.3, 2.3434, 201, unit=\"angstrom\")\n", "\n", "# Do not mask any pixels / voxels:\n", - "workflow = powder.with_pixel_mask_filenames(workflow, [])" + "workflow[PixelMaskFilenames] = []" ] }, { @@ -269,7 +269,7 @@ "workflow[DspacingBins] = sc.linspace(\"dspacing\", 0.3, 2.3434, 201, unit=\"angstrom\")\n", "\n", "# Do not mask any pixels / voxels:\n", - "workflow = powder.with_pixel_mask_filenames(workflow, [])" + "workflow[PixelMaskFilenames] = []" ] }, { @@ -381,7 +381,7 @@ "workflow[DspacingBins] = sc.linspace(\"dspacing\", 0.3, 2.3434, 201, unit=\"angstrom\")\n", "\n", "# Do not mask any pixels / voxels:\n", - "workflow = powder.with_pixel_mask_filenames(workflow, [])" + "workflow[PixelMaskFilenames] = []" ] }, { diff --git a/packages/essdiffraction/docs/user-guide/dream/dream-powder-reduction.ipynb b/packages/essdiffraction/docs/user-guide/dream/dream-powder-reduction.ipynb index 8811df9f0..a7a280469 100644 --- a/packages/essdiffraction/docs/user-guide/dream/dream-powder-reduction.ipynb +++ b/packages/essdiffraction/docs/user-guide/dream/dream-powder-reduction.ipynb @@ -96,7 +96,7 @@ "workflow[DspacingBins] = sc.linspace(\"dspacing\", 0.3, 2.3434, 201, unit=\"angstrom\")\n", "\n", "# Do not mask any pixels / voxels:\n", - "workflow = powder.with_pixel_mask_filenames(workflow, [])" + "workflow[PixelMaskFilenames] = []" ] }, { diff --git a/packages/essdiffraction/docs/user-guide/sns-instruments/POWGEN_data_reduction.ipynb b/packages/essdiffraction/docs/user-guide/sns-instruments/POWGEN_data_reduction.ipynb index 56d3c691e..525460aec 100644 --- a/packages/essdiffraction/docs/user-guide/sns-instruments/POWGEN_data_reduction.ipynb +++ b/packages/essdiffraction/docs/user-guide/sns-instruments/POWGEN_data_reduction.ipynb @@ -28,7 +28,6 @@ "import scippneutron as scn\n", "import scippneutron.io\n", "\n", - "from ess import powder\n", "from ess.snspowder import powgen\n", "import ess.snspowder.powgen.data # noqa: F401\n", "from ess.powder.types import *" @@ -84,7 +83,7 @@ "workflow[TwoThetaMask] = None\n", "workflow[WavelengthMask] = None\n", "# No pixel masks\n", - "workflow = powder.with_pixel_mask_filenames(workflow, [])" + "workflow[PixelMaskFilenames] = []" ] }, { diff --git a/packages/essdiffraction/src/ess/dream/parameters.py b/packages/essdiffraction/src/ess/dream/parameters.py index f82c8906e..db8b2ecd6 100644 --- a/packages/essdiffraction/src/ess/dream/parameters.py +++ b/packages/essdiffraction/src/ess/dream/parameters.py @@ -13,7 +13,7 @@ IntensityDspacingTwoTheta, MonitorFilename, NeXusDetectorName, - PixelMaskFilename, + PixelMaskFilenames, ReducedTofCIF, SampleRun, UncertaintyBroadcastMode, @@ -52,8 +52,8 @@ parameter_registry[MonitorFilename[VanadiumRun]] = FilenameParameter.from_type( MonitorFilename[VanadiumRun] ) -parameter_registry[PixelMaskFilename] = MultiFilenameParameter.from_type( - PixelMaskFilename, +parameter_registry[PixelMaskFilenames] = MultiFilenameParameter.from_type( + PixelMaskFilenames, ) parameter_registry[NeXusDetectorName] = StringParameter.from_type( NeXusDetectorName, default="mantle" diff --git a/packages/essdiffraction/src/ess/dream/workflows.py b/packages/essdiffraction/src/ess/dream/workflows.py index 149a5e8b4..f68905586 100644 --- a/packages/essdiffraction/src/ess/dream/workflows.py +++ b/packages/essdiffraction/src/ess/dream/workflows.py @@ -7,7 +7,6 @@ import scipp as sc import scippnexus as snx from ess.powder import providers as powder_providers -from ess.powder import with_pixel_mask_filenames from ess.powder.correction import RunNormalization, insert_run_normalization from ess.powder.types import ( AccumulatedProtonCharge, @@ -19,7 +18,6 @@ LookupTableFilename, LookupTableRelativeErrorThreshold, Measurement, - PixelMaskFilename, Position, ReducerSoftware, SampleRun, @@ -32,7 +30,6 @@ from scippneutron.metadata import Software from ess.reduce.nexus.types import DetectorBankSizes, NeXusName -from ess.reduce.parameter import parameter_mappers from ess.reduce.unwrap import GenericUnwrapWorkflow, WavelengthLutMode from ess.reduce.workflow import register_workflow @@ -149,8 +146,6 @@ def DreamWorkflow( prepare_reduced_empty_can_subtracted_tof_cif, ) -parameter_mappers[PixelMaskFilename] = with_pixel_mask_filenames - def default_parameters() -> dict: """Return the default reduction parameters shared by DREAM powder workflows.""" diff --git a/packages/essdiffraction/src/ess/powder/__init__.py b/packages/essdiffraction/src/ess/powder/__init__.py index 7fefdbbf0..6c69426f8 100644 --- a/packages/essdiffraction/src/ess/powder/__init__.py +++ b/packages/essdiffraction/src/ess/powder/__init__.py @@ -18,7 +18,6 @@ workflow, ) from .correction import RunNormalization -from .masking import with_pixel_mask_filenames try: __version__ = importlib.metadata.version("essdiffraction") @@ -51,6 +50,5 @@ "masking", "providers", "smoothing", - "with_pixel_mask_filenames", "workflow", ] diff --git a/packages/essdiffraction/src/ess/powder/masking.py b/packages/essdiffraction/src/ess/powder/masking.py index 7f64550b8..e10899ac8 100644 --- a/packages/essdiffraction/src/ess/powder/masking.py +++ b/packages/essdiffraction/src/ess/powder/masking.py @@ -4,15 +4,12 @@ Masking functions for the powder workflow. """ -from collections.abc import Iterable - -import numpy as np -import sciline import scipp as sc +from ess.reduce.masking import read_pixel_masks, to_pixel_masks + from .types import ( - MaskedDetectorIDs, - PixelMaskFilename, + LoadedPixelMasks, RunType, TofMask, TwoThetaMask, @@ -21,35 +18,20 @@ ) -def read_pixel_masks(filename: PixelMaskFilename) -> MaskedDetectorIDs: - """Read a pixel mask from a Scipp hdf5 file. - - Parameters - ---------- - filename: - Path to the hdf5 file. - """ - return MaskedDetectorIDs({filename: sc.io.load_hdf5(filename)}) - - def apply_masks( data: WavelengthDetector[RunType], - masked_pixel_ids: MaskedDetectorIDs, + pixel_masks: LoadedPixelMasks, tof_mask_func: TofMask, wavelength_mask_func: WavelengthMask, two_theta_mask_func: TwoThetaMask, ) -> sc.DataArray: """ """ out = data.copy(deep=False) - if len(masked_pixel_ids) > 0: + if len(pixel_masks) > 0: key = ( set(out.coords.keys()) & {"detector_number", "detector_id", "spectrum"} ).pop() - ids = out.coords[key] - for name, masked in masked_pixel_ids.items(): - mask = sc.zeros(sizes=ids.sizes, dtype="bool") - mask.values[np.isin(ids.values, masked.values)] = True - out.masks[name] = mask + out = out.assign_masks(to_pixel_masks(pixel_masks, out.coords[key])) for dim, mask in { "tof": tof_mask_func, @@ -71,31 +53,3 @@ def apply_masks( providers = (read_pixel_masks,) - - -def _merge(*dicts: dict) -> dict: - return {key: value for d in dicts for key, value in d.items()} - - -def with_pixel_mask_filenames( - workflow: sciline.Pipeline, masks: Iterable[str] -) -> sciline.Pipeline: - """ - Return modified workflow with pixel mask filenames set. - - Parameters - ---------- - workflow: - Workflow to modify. - masks: - List or tuple of pixel mask filenames to set. - """ - workflow = workflow.copy() - # Workaround bug in Cyclebane, which does not allow empty maps - if len(masks) == 0: - workflow[MaskedDetectorIDs] = MaskedDetectorIDs({}) - return workflow - workflow[MaskedDetectorIDs] = ( - workflow[MaskedDetectorIDs].map({PixelMaskFilename: masks}).reduce(func=_merge) - ) - return workflow diff --git a/packages/essdiffraction/src/ess/powder/types.py b/packages/essdiffraction/src/ess/powder/types.py index 062533457..86f96ade4 100644 --- a/packages/essdiffraction/src/ess/powder/types.py +++ b/packages/essdiffraction/src/ess/powder/types.py @@ -17,6 +17,7 @@ from scippneutron.io import cif from scippneutron.metadata import Person, Software +from ess.reduce import masking as masking_t from ess.reduce.nexus import types as reduce_t from ess.reduce.uncertainty import UncertaintyBroadcastMode as _UncertaintyBroadcastMode from ess.reduce.unwrap import types as unwrap_t @@ -177,9 +178,6 @@ class CorrectedDetector(sciline.Scope[RunType, sc.DataArray], sc.DataArray): """ -MaskedDetectorIDs = NewType("MaskedDetectorIDs", dict[str, sc.Variable]) -"""1-D variable listing all masked detector IDs.""" - CaveMonitorPosition = NewType("CaveMonitorPosition", sc.Variable) """Position of DREAM's cave monitor.""" @@ -204,8 +202,8 @@ class NormalizedDspacing(sciline.Scope[RunType, sc.DataArray], sc.DataArray): """Detector data in d-spacing - 2theta space normalized by monitor/proton charge.""" -PixelMaskFilename = NewType("PixelMaskFilename", str) -"""Filename of a pixel mask.""" +PixelMaskFilenames = masking_t.PixelMaskFilenames +LoadedPixelMasks = masking_t.LoadedPixelMasks class RawDataAndMetadata(sciline.Scope[RunType, sc.DataGroup], sc.DataGroup): diff --git a/packages/essdiffraction/src/ess/powder/workflow.py b/packages/essdiffraction/src/ess/powder/workflow.py index a0ca3828c..3e71ad462 100644 --- a/packages/essdiffraction/src/ess/powder/workflow.py +++ b/packages/essdiffraction/src/ess/powder/workflow.py @@ -3,7 +3,7 @@ from .types import ( CorrectedDetector, DspacingDetector, - MaskedDetectorIDs, + LoadedPixelMasks, RunType, TofMask, TwoThetaMask, @@ -13,14 +13,14 @@ def add_masks_and_corrections( da: DspacingDetector[RunType], - masked_pixel_ids: MaskedDetectorIDs, + pixel_masks: LoadedPixelMasks, tof_mask_func: TofMask, wavelength_mask_func: WavelengthMask, two_theta_mask_func: TwoThetaMask, ) -> CorrectedDetector[RunType]: masked = apply_masks( data=da, - masked_pixel_ids=masked_pixel_ids, + pixel_masks=pixel_masks, tof_mask_func=tof_mask_func, wavelength_mask_func=wavelength_mask_func, two_theta_mask_func=two_theta_mask_func, diff --git a/packages/essdiffraction/tests/beer/mcstas_reduction_test.py b/packages/essdiffraction/tests/beer/mcstas_reduction_test.py index f866bf2bd..4060e0f13 100644 --- a/packages/essdiffraction/tests/beer/mcstas_reduction_test.py +++ b/packages/essdiffraction/tests/beer/mcstas_reduction_test.py @@ -49,7 +49,7 @@ ElasticCoordTransformGraph, EmptyDetector, KeepEvents, - MaskedDetectorIDs, + LoadedPixelMasks, NormalizedDspacing, QDetector, SampleRun, @@ -155,7 +155,7 @@ def test_modulation_workflows_can_normalize(factory): wf[DetectorBank] = DetectorBank.north wf[DHKLList] = silicon_peaks_array() wf[DspacingBins] = sc.linspace('dspacing', 0.8, 2.2, 31, unit='angstrom') - wf[MaskedDetectorIDs] = MaskedDetectorIDs({}) + wf[LoadedPixelMasks] = LoadedPixelMasks({}) wf[KeepEvents[SampleRun]] = KeepEvents[SampleRun](True) wf[UncertaintyBroadcastMode] = UncertaintyBroadcastMode.drop wf[TofMask] = None diff --git a/packages/essdiffraction/tests/dream/geant4_reduction_test.py b/packages/essdiffraction/tests/dream/geant4_reduction_test.py index 779127038..30b327b00 100644 --- a/packages/essdiffraction/tests/dream/geant4_reduction_test.py +++ b/packages/essdiffraction/tests/dream/geant4_reduction_test.py @@ -33,6 +33,7 @@ MonitorFilename, NeXusDetectorName, OutFilename, + PixelMaskFilenames, ReducedTofCIF, SampleRun, TofMask, @@ -53,6 +54,7 @@ params = { + PixelMaskFilenames: [], Filename[SampleRun]: dream.data.simulated_diamond_sample(small=True), Filename[VanadiumRun]: dream.data.simulated_vanadium_sample(small=True), Filename[EmptyCanRun]: dream.data.simulated_empty_can(small=True), @@ -95,9 +97,7 @@ def _make_workflow( @pytest.mark.parametrize(*PARAMETRIZATION) def test_pipeline_can_compute_dspacing_result(detector_name): - workflow = powder.with_pixel_mask_filenames( - _make_workflow(detector_name=detector_name), [] - ) + workflow = _make_workflow(detector_name=detector_name) result = workflow.compute(EmptyCanSubtractedIofDspacing) assert result.sizes == {'dspacing': len(params[DspacingBins]) - 1} assert sc.identical(result.coords['dspacing'], params[DspacingBins]) @@ -108,7 +108,6 @@ def test_pipeline_can_compute_dspacing_result_without_empty_can(detector_name): workflow = _make_workflow(detector_name=detector_name) workflow[Filename[EmptyCanRun]] = None workflow[MonitorFilename[EmptyCanRun]] = None - workflow = powder.with_pixel_mask_filenames(workflow, []) result = workflow.compute(IntensityDspacing[SampleRun]) assert result.sizes == {'dspacing': len(params[DspacingBins]) - 1} assert sc.identical(result.coords['dspacing'], params[DspacingBins]) @@ -134,7 +133,6 @@ def test_pipeline_can_compute_dspacing_result_different_lookup_tables( workflow[unwrap.NumberOfSimulatedNeutrons] = 200_000 workflow[unwrap.SimulationSeed] = 555 - workflow = powder.with_pixel_mask_filenames(workflow, []) result = workflow.compute(EmptyCanSubtractedIofDspacing) assert result.sizes == {'dspacing': len(params[DspacingBins]) - 1} assert sc.identical(result.coords['dspacing'], params[DspacingBins]) @@ -149,7 +147,6 @@ def test_pipeline_can_compute_dspacing_result_with_different_norm( workflow = _make_workflow(detector_name=detector_name, run_norm=norm) workflow[KeepEvents[SampleRun]] = KeepEvents[SampleRun](keep_events) workflow[KeepEvents[VanadiumRun]] = KeepEvents[VanadiumRun](keep_events) - workflow = powder.with_pixel_mask_filenames(workflow, []) result = workflow.compute(IntensityDspacing[SampleRun]) assert result.sizes == {'dspacing': len(params[DspacingBins]) - 1} assert sc.identical(result.coords['dspacing'], params[DspacingBins]) @@ -166,7 +163,6 @@ def test_pipeline_normalizes_and_subtracts_empty_can_as_expected(detector_name) workflow[FocussedDataDspacing[VanadiumRun]] = vanadium workflow[FocussedDataDspacing[EmptyCanRun]] = empty_can workflow[UncertaintyBroadcastMode] = UncertaintyBroadcastMode.drop - workflow = powder.with_pixel_mask_filenames(workflow, []) result = workflow.compute(EmptyCanSubtractedIofDspacing) subtracted = sample.bins.concatenate(-empty_can) @@ -180,9 +176,7 @@ def test_pipeline_normalizes_and_subtracts_empty_can_as_expected(detector_name) @pytest.mark.parametrize(*PARAMETRIZATION) def test_workflow_is_deterministic(detector_name): - workflow = powder.with_pixel_mask_filenames( - _make_workflow(detector_name=detector_name), [] - ) + workflow = _make_workflow(detector_name=detector_name) # This is Sciline's default scheduler, but we want to be explicit here scheduler = sciline.scheduler.DaskScheduler() graph = workflow.get(IntensityTof, scheduler=scheduler) @@ -193,9 +187,7 @@ def test_workflow_is_deterministic(detector_name): @pytest.mark.parametrize(*PARAMETRIZATION) def test_pipeline_can_compute_intermediate_results(detector_name): - workflow = powder.with_pixel_mask_filenames( - _make_workflow(detector_name=detector_name), [] - ) + workflow = _make_workflow(detector_name=detector_name) results = workflow.compute((CorrectedDetector[SampleRun], NeXusDetectorName)) result = results[CorrectedDetector[SampleRun]] @@ -214,7 +206,6 @@ def test_pipeline_group_by_two_theta(detector_name): ) workflow = _make_workflow(detector_name=detector_name) workflow[TwoThetaBins] = two_theta_bins - workflow = powder.with_pixel_mask_filenames(workflow, []) result = workflow.compute(IntensityDspacingTwoTheta[SampleRun]) assert result.sizes['two_theta'] == 16 assert result.sizes['dspacing'] == len(params[DspacingBins]) - 1 @@ -228,7 +219,6 @@ def test_pipeline_wavelength_masking(detector_name): wmin = sc.scalar(0.18, unit="angstrom") wmax = sc.scalar(0.21, unit="angstrom") workflow[WavelengthMask] = lambda x: (x > wmin) & (x < wmax) - workflow = powder.with_pixel_mask_filenames(workflow, []) masked_sample = workflow.compute(CorrectedDetector[SampleRun]) assert 'wavelength' in masked_sample.bins.masks sum_in_masked_region = ( @@ -248,7 +238,6 @@ def test_pipeline_two_theta_masking(detector_name): tmin = sc.scalar(1.0, unit="rad") tmax = sc.scalar(1.2, unit="rad") workflow[TwoThetaMask] = lambda x: (x > tmin) & (x < tmax) - workflow = powder.with_pixel_mask_filenames(workflow, []) masked_sample = workflow.compute(CorrectedDetector[SampleRun]) assert 'two_theta' in masked_sample.masks sum_in_masked_region = ( @@ -262,9 +251,7 @@ def test_pipeline_two_theta_masking(detector_name): @pytest.mark.parametrize(*PARAMETRIZATION) def test_pipeline_can_save_data(detector_name): - workflow = powder.with_pixel_mask_filenames( - _make_workflow(detector_name=detector_name), [] - ) + workflow = _make_workflow(detector_name=detector_name) result = workflow.compute(ReducedTofCIF) buffer = io.StringIO() @@ -296,7 +283,6 @@ def test_pipeline_save_data_to_disk(output_folder: Path): wf[Filename[VanadiumRun]] = dream.data.simulated_vanadium_sample(small=False) wf[Filename[EmptyCanRun]] = dream.data.simulated_empty_can(small=False) wf[DspacingBins] = sc.linspace('dspacing', 0.3, 2.3434, 2001, unit='angstrom') - wf = powder.with_pixel_mask_filenames(wf, []) result = wf.compute(ReducedTofCIF) result.comment = """This file was generated with the DREAM data reduction user guide diff --git a/packages/essdiffraction/tests/powder/masking_test.py b/packages/essdiffraction/tests/powder/masking_test.py new file mode 100644 index 000000000..eb7f2cc25 --- /dev/null +++ b/packages/essdiffraction/tests/powder/masking_test.py @@ -0,0 +1,31 @@ +# SPDX-License-Identifier: BSD-3-Clause +# Copyright (c) 2026 Scipp contributors (https://github.com/scipp) +from pathlib import Path + +import scipp as sc +from ess.powder.masking import apply_masks, read_pixel_masks +from ess.powder.types import PixelMaskFilenames + + +def test_apply_masks_masks_pixels_listed_in_file(tmp_path: Path) -> None: + path = tmp_path / 'broken.h5' + masked = sc.array(dims=['id'], values=[2], unit=None, dtype='int32') + sc.io.save_hdf5(masked, path) + data = sc.DataArray( + sc.ones(sizes={'detector_number': 3}), + coords={ + 'detector_number': sc.array( + dims=['detector_number'], values=[1, 2, 3], unit=None + ) + }, + ) + + result = apply_masks( + data=data, + pixel_masks=read_pixel_masks(PixelMaskFilenames([path])), + tof_mask_func=None, + wavelength_mask_func=None, + two_theta_mask_func=None, + ) + + assert result.masks['broken.h5'].values.tolist() == [False, True, False] diff --git a/packages/essdiffraction/tests/snspowder/powgen/powgen_reduction_test.py b/packages/essdiffraction/tests/snspowder/powgen/powgen_reduction_test.py index 3ff09ac1c..679d596a8 100644 --- a/packages/essdiffraction/tests/snspowder/powgen/powgen_reduction_test.py +++ b/packages/essdiffraction/tests/snspowder/powgen/powgen_reduction_test.py @@ -5,7 +5,6 @@ import pytest import sciline import scipp as sc -from ess import powder from ess.powder.types import ( CalibrationFilename, CorrectedDetector, @@ -17,6 +16,7 @@ IntensityDspacingTwoTheta, KeepEvents, NeXusDetectorName, + PixelMaskFilenames, SampleRun, TofMask, TwoThetaBins, @@ -42,6 +42,7 @@ def params(): KeepEvents[SampleRun]: KeepEvents[SampleRun](True), KeepEvents[VanadiumRun]: KeepEvents[VanadiumRun](False), NeXusDetectorName: "powgen_detector", + PixelMaskFilenames: [], Filename[SampleRun]: powgen.data.powgen_tutorial_sample_file(small=True), Filename[VanadiumRun]: powgen.data.powgen_tutorial_vanadium_file(small=True), CalibrationFilename: powgen.data.powgen_tutorial_calibration_file(small=True), @@ -66,7 +67,6 @@ def test_can_create_pipeline(providers, params): def test_pipeline_can_compute_dspacing_result(providers, params): pipeline = sciline.Pipeline(providers, params=params) - pipeline = powder.with_pixel_mask_filenames(pipeline, []) result = pipeline.compute(IntensityDspacing[SampleRun]) assert result.sizes == { 'dspacing': len(params[DspacingBins]) - 1, @@ -77,7 +77,6 @@ def test_pipeline_can_compute_dspacing_result(providers, params): def test_pipeline_can_compute_dspacing_result_without_calibration(providers, params): params[CalibrationFilename] = None pipeline = sciline.Pipeline(providers, params=params) - pipeline = powder.with_pixel_mask_filenames(pipeline, []) result = pipeline.compute(IntensityDspacing[SampleRun]) assert result.sizes == { 'dspacing': len(params[DspacingBins]) - 1, @@ -87,12 +86,10 @@ def test_pipeline_can_compute_dspacing_result_without_calibration(providers, par def test_pipeline_compare_with_and_without_calibration(providers, params): pipeline = sciline.Pipeline(providers, params=params) - pipeline = powder.with_pixel_mask_filenames(pipeline, []) result_w_cal = pipeline.compute(IntensityDspacing[SampleRun]) params[CalibrationFilename] = None pipeline = sciline.Pipeline(providers, params=params) - pipeline = powder.with_pixel_mask_filenames(pipeline, []) result_wo_cal = pipeline.compute(IntensityDspacing[SampleRun]) assert sc.identical( @@ -103,7 +100,6 @@ def test_pipeline_compare_with_and_without_calibration(providers, params): def test_workflow_is_deterministic(providers, params): pipeline = sciline.Pipeline(providers, params=params) - pipeline = powder.with_pixel_mask_filenames(pipeline, []) # This is Sciline's default scheduler, but we want to be explicit here scheduler = sciline.scheduler.DaskScheduler() graph = pipeline.get(IntensityDspacing[SampleRun], scheduler=scheduler) @@ -114,7 +110,6 @@ def test_workflow_is_deterministic(providers, params): def test_pipeline_can_compute_intermediate_results(providers, params): pipeline = sciline.Pipeline(providers, params=params) - pipeline = powder.with_pixel_mask_filenames(pipeline, []) result = pipeline.compute(CorrectedDetector[SampleRun]) assert set(result.dims) == {'bank', 'column', 'row'} @@ -124,7 +119,6 @@ def test_pipeline_group_by_two_theta(providers, params): dim='two_theta', unit='deg', start=25.0, stop=90.0, num=16 ).to(unit='rad') pipeline = sciline.Pipeline(providers, params=params) - pipeline = powder.with_pixel_mask_filenames(pipeline, []) result = pipeline.compute(IntensityDspacingTwoTheta[SampleRun]) assert result.sizes == { 'two_theta': 15, @@ -139,7 +133,6 @@ def test_pipeline_wavelength_masking(providers, params): wmax = sc.scalar(0.21, unit="angstrom") params[WavelengthMask] = lambda x: (x > wmin) & (x < wmax) pipeline = sciline.Pipeline(providers, params=params) - pipeline = powder.with_pixel_mask_filenames(pipeline, []) masked_sample = pipeline.compute(CorrectedDetector[SampleRun]) assert 'wavelength' in masked_sample.bins.masks sum_in_masked_region = ( @@ -160,7 +153,6 @@ def test_pipeline_two_theta_masking(providers, params): tmax = sc.scalar(1.0, unit="rad") params[TwoThetaMask] = lambda x: (x > tmin) & (x < tmax) pipeline = sciline.Pipeline(providers, params=params) - pipeline = powder.with_pixel_mask_filenames(pipeline, []) masked_sample = pipeline.compute(CorrectedDetector[SampleRun]) assert 'two_theta' in masked_sample.masks sum_in_masked_region = ( @@ -181,7 +173,6 @@ def test_use_workflow_helper(params): workflow = powgen.PowgenWorkflow() for key, value in params.items(): workflow[key] = value - workflow = powder.with_pixel_mask_filenames(workflow, []) result = workflow.compute(IntensityDspacing[SampleRun]) assert result.sizes == { 'dspacing': len(params[DspacingBins]) - 1, diff --git a/packages/essdiffraction/tools/docs/beer-thumbnails.ipynb b/packages/essdiffraction/tools/docs/beer-thumbnails.ipynb index 3cdbed313..8060245ed 100644 --- a/packages/essdiffraction/tools/docs/beer-thumbnails.ipynb +++ b/packages/essdiffraction/tools/docs/beer-thumbnails.ipynb @@ -31,7 +31,7 @@ " EmptyCanSubtractedIofDspacing,\n", " Filename,\n", " KeepEvents,\n", - " MaskedDetectorIDs,\n", + " LoadedPixelMasks,\n", " SampleRun,\n", " TofMask,\n", " TwoThetaMask,\n", @@ -153,7 +153,7 @@ "for run_type in (SampleRun, VanadiumRun, EmptyCanRun):\n", " powder_workflow[KeepEvents[run_type]] = KeepEvents(True)\n", "\n", - "powder_workflow[MaskedDetectorIDs] = MaskedDetectorIDs({})\n", + "powder_workflow[LoadedPixelMasks] = LoadedPixelMasks({})\n", "powder_workflow[UncertaintyBroadcastMode] = UncertaintyBroadcastMode.drop\n", "powder_workflow[TwoThetaMask] = None\n", "powder_workflow[TofMask] = None\n", diff --git a/packages/essdiffraction/tools/docs/dream-thumbnails.ipynb b/packages/essdiffraction/tools/docs/dream-thumbnails.ipynb index fd96527e3..9c1524be5 100644 --- a/packages/essdiffraction/tools/docs/dream-thumbnails.ipynb +++ b/packages/essdiffraction/tools/docs/dream-thumbnails.ipynb @@ -62,7 +62,7 @@ "workflow[DspacingBins] = sc.linspace(\"dspacing\", 0.3, 2.3434, 201, unit=\"angstrom\")\n", "\n", "# Do not mask any pixels / voxels:\n", - "workflow = powder.with_pixel_mask_filenames(workflow, [])" + "workflow[PixelMaskFilenames] = []" ] }, { diff --git a/packages/essreduce/docs/api-reference/index.md b/packages/essreduce/docs/api-reference/index.md index b80fdd093..9dbd2179b 100644 --- a/packages/essreduce/docs/api-reference/index.md +++ b/packages/essreduce/docs/api-reference/index.md @@ -30,6 +30,7 @@ diagnostics live logging + masking nexus normalization polarization diff --git a/packages/essreduce/docs/user-guide/polarization/zoom.ipynb b/packages/essreduce/docs/user-guide/polarization/zoom.ipynb index 6d4087e8a..605c76e25 100644 --- a/packages/essreduce/docs/user-guide/polarization/zoom.ipynb +++ b/packages/essreduce/docs/user-guide/polarization/zoom.ipynb @@ -433,7 +433,7 @@ "\n", " # TODO Are these masks correct? Provide correct XML files!\n", " masks = isis.data.zoom_tutorial_mask_filenames()\n", - " wf = sans.with_pixel_mask_filenames(wf, masks)\n", + " wf[PixelMaskFilenames] = masks\n", "\n", " for key, value in params.items():\n", " wf[key] = value\n", diff --git a/packages/essreduce/src/ess/reduce/__init__.py b/packages/essreduce/src/ess/reduce/__init__.py index 091d28e0b..dc5516f1f 100644 --- a/packages/essreduce/src/ess/reduce/__init__.py +++ b/packages/essreduce/src/ess/reduce/__init__.py @@ -16,6 +16,7 @@ __name__, submodules=[ "diagnostics", + "masking", "nexus", "normalization", "polarization", diff --git a/packages/essreduce/src/ess/reduce/masking.py b/packages/essreduce/src/ess/reduce/masking.py new file mode 100644 index 000000000..e58afd0e2 --- /dev/null +++ b/packages/essreduce/src/ess/reduce/masking.py @@ -0,0 +1,122 @@ +# SPDX-License-Identifier: BSD-3-Clause +# Copyright (c) 2026 Scipp contributors (https://github.com/scipp) +"""Pixel masks stored in files. + +Reading is independent of runs and detector banks, so a workflow reads its mask files +once. Converting the file content to masks for a detector is a separate step. +""" + +from collections.abc import Sequence +from pathlib import Path +from typing import NewType + +import numpy as np +import scipp as sc + +PixelMaskFilenames = NewType('PixelMaskFilenames', Sequence[str | Path]) +"""Files with pixel masks to apply. Use an empty sequence to apply none.""" + +LoadedPixelMasks = NewType('LoadedPixelMasks', dict[str, sc.Variable]) +"""Masked detector IDs from each mask file, by mask name.""" + + +def read_pixel_masks(filenames: PixelMaskFilenames) -> LoadedPixelMasks: + """Read pixel masks from files. + + A mask is named after its file, without the directory, since directories differ + between hosts. The following formats are supported, selected by file suffix: + + - ``.xml``: Masked detector IDs in the ISIS format, + see :py:func:`read_xml_masked_detector_ids`. + - ``.h5``, ``.hdf``, ``.hdf5``: A scipp variable of masked detector IDs, as written + by :py:func:`scipp.io.save_hdf5`. + + Parameters + ---------- + filenames: + Paths to the mask files. + + Raises + ------ + ValueError + If a file has an unsupported suffix or content, or two files give the same mask + name. One mask would replace the other otherwise. + """ + masks = {} + for filename in map(Path, filenames): + if filename.name in masks: + raise ValueError(f"Several mask files have the name '{filename.name}'.") + masks[filename.name] = _read_mask_file(filename) + return LoadedPixelMasks(masks) + + +def to_pixel_masks( + loaded: LoadedPixelMasks, ids: sc.Variable +) -> dict[str, sc.Variable]: + """Create a boolean mask per loaded mask, for the pixels of a detector. + + Parameters + ---------- + loaded: + Masks read from files. + ids: + Detector IDs of the pixels. The masks have the dimensions of this variable. + """ + return { + name: sc.array(dims=ids.dims, values=np.isin(ids.values, masked.values)) + for name, masked in loaded.items() + } + + +def read_xml_masked_detector_ids(filename: str | Path) -> sc.Variable: + """Read a pixel mask from an ISIS XML file. + + The format is as follows, where the detids are inclusive ranges of detector IDs: + + .. code-block:: xml + + + + + 1400203-1400218,1401199,1402190-1402223 + + + + Parameters + ---------- + filename: + Path to the XML file. + """ + import xml.etree.ElementTree as ET # nosec + + tree = ET.parse(filename) # noqa: S314 + root = tree.getroot() + + masked_detids = [] + for group in root.findall('group'): + for detids in group.findall('detids'): + for detid in detids.text.split(','): + detid = detid.strip() + if '-' in detid: + start, stop = detid.split('-') + masked_detids += list(range(int(start), int(stop) + 1)) + else: + masked_detids.append(int(detid)) + + return sc.array( + dims=['detector_id'], values=masked_detids, unit=None, dtype='int32' + ) + + +def _read_mask_file(filename: Path) -> sc.Variable: + if filename.suffix == '.xml': + return read_xml_masked_detector_ids(filename) + if filename.suffix in ('.h5', '.hdf', '.hdf5'): + return _validate_ids(sc.io.load_hdf5(filename), filename) + raise ValueError(f"Unsupported suffix of mask file '{filename}'.") + + +def _validate_ids(content: object, filename: Path) -> sc.Variable: + if isinstance(content, sc.Variable) and content.dtype in ('int32', 'int64'): + return content + raise ValueError(f"Mask file '{filename}' does not hold integer detector IDs.") diff --git a/packages/essreduce/tests/masking_test.py b/packages/essreduce/tests/masking_test.py new file mode 100644 index 000000000..2e7bc3ac2 --- /dev/null +++ b/packages/essreduce/tests/masking_test.py @@ -0,0 +1,84 @@ +# SPDX-License-Identifier: BSD-3-Clause +# Copyright (c) 2026 Scipp contributors (https://github.com/scipp) +from pathlib import Path + +import pytest +import scipp as sc + +from ess.reduce.masking import ( + LoadedPixelMasks, + PixelMaskFilenames, + read_pixel_masks, + to_pixel_masks, +) + + +def _write_xml(path: Path, detids: str) -> Path: + path.write_text( + '\n' + f'{detids}' + ) + return path + + +def _write_ids(path: Path, values: list[int]) -> Path: + sc.io.save_hdf5( + sc.array(dims=['detector_id'], values=values, unit=None, dtype='int32'), path + ) + return path + + +def _read(*paths: Path) -> LoadedPixelMasks: + return read_pixel_masks(PixelMaskFilenames(paths)) + + +def test_read_no_files_gives_no_masks() -> None: + assert read_pixel_masks(PixelMaskFilenames([])) == {} + + +def test_read_xml_expands_ranges(tmp_path: Path) -> None: + masks = _read(_write_xml(tmp_path / 'a.xml', '1-3, 7')) + assert masks['a.xml'].values.tolist() == [1, 2, 3, 7] + + +def test_read_hdf5_detector_ids(tmp_path: Path) -> None: + masks = _read(_write_ids(tmp_path / 'a.h5', [4, 5])) + assert masks['a.h5'].values.tolist() == [4, 5] + + +def test_read_combines_formats_and_names_masks_without_directory( + tmp_path: Path, +) -> None: + masks = _read( + _write_xml(tmp_path / 'a.xml', '1'), _write_ids(tmp_path / 'b.hdf5', [2]) + ) + assert list(masks) == ['a.xml', 'b.hdf5'] + + +def test_read_raises_if_two_files_have_same_name(tmp_path: Path) -> None: + (tmp_path / 'x').mkdir() + (tmp_path / 'y').mkdir() + first = _write_xml(tmp_path / 'x' / 'a.xml', '1') + second = _write_xml(tmp_path / 'y' / 'a.xml', '2') + with pytest.raises(ValueError, match=r'a\.xml'): + _read(first, second) + + +def test_read_raises_on_unsupported_suffix(tmp_path: Path) -> None: + with pytest.raises(ValueError, match='Unsupported suffix'): + _read(tmp_path / 'a.txt') + + +def test_read_raises_if_hdf5_does_not_hold_integers(tmp_path: Path) -> None: + path = tmp_path / 'a.h5' + sc.io.save_hdf5(sc.array(dims=['x'], values=[1.5]), path) + with pytest.raises(ValueError, match='integer detector IDs'): + _read(path) + + +def test_to_pixel_masks_keeps_shape_of_ids() -> None: + ids = sc.array(dims=['tube', 'pixel'], values=[[1, 2], [3, 4]], unit=None) + masked = sc.array(dims=['detector_id'], values=[2, 3, 99], unit=None) + masks = to_pixel_masks(LoadedPixelMasks({'m': masked}), ids) + expected = sc.array(dims=['tube', 'pixel'], values=[[False, True], [True, False]]) + assert sc.identical(masks['m'], expected) diff --git a/packages/esssans/docs/api-reference/index.md b/packages/esssans/docs/api-reference/index.md index 3c28e9713..4fe5127c8 100644 --- a/packages/esssans/docs/api-reference/index.md +++ b/packages/esssans/docs/api-reference/index.md @@ -21,7 +21,6 @@ providers with_background_runs with_banks - with_pixel_mask_filenames with_sample_runs ``` diff --git a/packages/esssans/docs/user-guide/isis/zoom.ipynb b/packages/esssans/docs/user-guide/isis/zoom.ipynb index 4d7a1b2af..e42737014 100644 --- a/packages/esssans/docs/user-guide/isis/zoom.ipynb +++ b/packages/esssans/docs/user-guide/isis/zoom.ipynb @@ -146,7 +146,7 @@ "workflow[isis.SampleOffset] = sc.vector([0.0, 0.0, 0.11], unit='m')\n", "workflow[isis.DetectorBankOffset] = sc.vector([0.0, 0.0, 0.5], unit='m')\n", "masks = isis.data.zoom_tutorial_mask_filenames()\n", - "workflow = sans.with_pixel_mask_filenames(workflow, masks)" + "workflow[PixelMaskFilenames] = masks" ] }, { diff --git a/packages/esssans/docs/user-guide/loki/loki-direct-beam.ipynb b/packages/esssans/docs/user-guide/loki/loki-direct-beam.ipynb index 26c8b9f99..3dcb8e038 100644 --- a/packages/esssans/docs/user-guide/loki/loki-direct-beam.ipynb +++ b/packages/esssans/docs/user-guide/loki/loki-direct-beam.ipynb @@ -83,9 +83,7 @@ "metadata": {}, "outputs": [], "source": [ - "workflow = sans.with_pixel_mask_filenames(\n", - " workflow, masks=loki.data.loki_tutorial_mask_filenames()\n", - ")\n", + "workflow[PixelMaskFilenames] = loki.data.loki_tutorial_mask_filenames()\n", "workflow[NeXusDetectorName] = 'larmor_detector'" ] }, diff --git a/packages/esssans/docs/user-guide/loki/loki-iofq.ipynb b/packages/esssans/docs/user-guide/loki/loki-iofq.ipynb index 90988ed71..8170da252 100644 --- a/packages/esssans/docs/user-guide/loki/loki-iofq.ipynb +++ b/packages/esssans/docs/user-guide/loki/loki-iofq.ipynb @@ -55,9 +55,7 @@ "workflow = loki.LokiAtLarmorWorkflow()\n", "\n", "# Load masks from files\n", - "workflow = sans.with_pixel_mask_filenames(\n", - " workflow, masks=loki.data.loki_tutorial_mask_filenames()\n", - ")\n", + "workflow[PixelMaskFilenames] = loki.data.loki_tutorial_mask_filenames()\n", "# Set detector bank name: in this case there is only one bank\n", "workflow[NeXusDetectorName] = \"larmor_detector\"\n", "\n", diff --git a/packages/esssans/src/ess/isissans/data.py b/packages/esssans/src/ess/isissans/data.py index c38168f7c..ed87b7934 100644 --- a/packages/esssans/src/ess/isissans/data.py +++ b/packages/esssans/src/ess/isissans/data.py @@ -7,7 +7,7 @@ DirectBeamFilename, EmptyBeamRun, Filename, - PixelMaskFilename, + PixelMaskFilenames, SampleRun, ) @@ -106,13 +106,15 @@ def zoom_tutorial_empty_beam_run() -> Filename[EmptyBeamRun]: return Filename[EmptyBeamRun](_zoom_registry.get_path('ZOOM00034787.nxs.h5')) -def zoom_tutorial_mask_filenames() -> list[PixelMaskFilename]: - return [ - PixelMaskFilename(_zoom_registry.get_path('andru_test.xml')), - PixelMaskFilename(_zoom_registry.get_path('left_beg_18_2.xml')), - PixelMaskFilename(_zoom_registry.get_path('right_beg_18_2.xml')), - PixelMaskFilename(_zoom_registry.get_path('small_bs_232.xml')), - PixelMaskFilename(_zoom_registry.get_path('small_BS_31032023.xml')), - PixelMaskFilename(_zoom_registry.get_path('tube_1120_bottom.xml')), - PixelMaskFilename(_zoom_registry.get_path('tubes_beg_18_2.xml')), - ] +def zoom_tutorial_mask_filenames() -> PixelMaskFilenames: + return PixelMaskFilenames( + [ + _zoom_registry.get_path('andru_test.xml'), + _zoom_registry.get_path('left_beg_18_2.xml'), + _zoom_registry.get_path('right_beg_18_2.xml'), + _zoom_registry.get_path('small_bs_232.xml'), + _zoom_registry.get_path('small_BS_31032023.xml'), + _zoom_registry.get_path('tube_1120_bottom.xml'), + _zoom_registry.get_path('tubes_beg_18_2.xml'), + ] + ) diff --git a/packages/esssans/src/ess/isissans/sans2d.py b/packages/esssans/src/ess/isissans/sans2d.py index 119ad1626..253ff1240 100644 --- a/packages/esssans/src/ess/isissans/sans2d.py +++ b/packages/esssans/src/ess/isissans/sans2d.py @@ -54,9 +54,8 @@ def to_detector_masks( ) -> DetectorMasks: """Gather detector masks. - Unlike :py:func:`ess.sans.masking.to_detector_mask`, this function does not rely on - mapping using a table of mask filenames. Instead it directly returns a dictionary - if multiple masks. + Unlike :py:func:`ess.sans.masking.to_detector_mask`, this function does not use + mask files. Instead it directly returns a dictionary of multiple masks. Parameters ---------- diff --git a/packages/esssans/src/ess/isissans/zoom.py b/packages/esssans/src/ess/isissans/zoom.py index 3b5cddc7c..1d0785940 100644 --- a/packages/esssans/src/ess/isissans/zoom.py +++ b/packages/esssans/src/ess/isissans/zoom.py @@ -5,7 +5,6 @@ import sciline import scipp as sc from ess.sans import SansWorkflow -from ess.sans.io import read_xml_detector_masking from ess.sans.parameters import typical_outputs from ess.sans.types import ( Filename, @@ -56,7 +55,6 @@ def ZoomWorkflow() -> sciline.Pipeline: workflow.insert(provider) for key, param in default_parameters().items(): workflow[key] = param - workflow.insert(read_xml_detector_masking) workflow.typical_outputs = typical_outputs return workflow diff --git a/packages/esssans/src/ess/loki/data.py b/packages/esssans/src/ess/loki/data.py index dc58765fe..b491d0561 100644 --- a/packages/esssans/src/ess/loki/data.py +++ b/packages/esssans/src/ess/loki/data.py @@ -6,7 +6,7 @@ BackgroundRun, DirectBeamFilename, Filename, - PixelMaskFilename, + PixelMaskFilenames, SampleRun, TransmissionRun, ) @@ -154,11 +154,9 @@ def loki_tutorial_deut_sds_transmission_run() -> Filename[TransmissionRun[Sample ) -def loki_tutorial_mask_filenames() -> list[PixelMaskFilename]: +def loki_tutorial_mask_filenames() -> PixelMaskFilenames: """List of pixel mask filenames for the LoKI@Larmor detector test experiment.""" - return [ - PixelMaskFilename(_registry.get_path('mask_new_July2022.xml')), - ] + return PixelMaskFilenames([_registry.get_path('mask_new_July2022.xml')]) def loki_tutorial_poly_gauss_I0() -> Path: diff --git a/packages/esssans/src/ess/loki/larmor_workflow.py b/packages/esssans/src/ess/loki/larmor_workflow.py index 0ee8d66e4..35af21422 100644 --- a/packages/esssans/src/ess/loki/larmor_workflow.py +++ b/packages/esssans/src/ess/loki/larmor_workflow.py @@ -8,7 +8,6 @@ import scipp as sc import scippnexus as snx from ess import sans -from ess.sans.io import read_xml_detector_masking from ess.sans.parameters import typical_outputs from scippneutron.conversion.graph import beamline, tof @@ -28,7 +27,7 @@ NeXusDetectorName, NeXusMonitorName, NonBackgroundWavelengthRange, - PixelMaskFilename, + PixelMaskFilenames, PixelShapePath, Position, RawDetector, @@ -141,7 +140,6 @@ def larmor_monitor_coord_transform_graph( larmor_monitor_to_wavelength, larmor_detector_coord_transform_graph, larmor_monitor_coord_transform_graph, - read_xml_detector_masking, ) @@ -178,7 +176,7 @@ def LokiAtLarmorTutorialWorkflow() -> sciline.Pipeline: workflow = LokiAtLarmorWorkflow() - workflow[PixelMaskFilename] = list(map(str, data.loki_tutorial_mask_filenames())) + workflow[PixelMaskFilenames] = data.loki_tutorial_mask_filenames() workflow[Filename[SampleRun]] = str(data.loki_tutorial_sample_run_60339()) workflow[Filename[BackgroundRun]] = str(data.loki_tutorial_background_run_60393()) workflow[Filename[TransmissionRun[SampleRun]]] = str( diff --git a/packages/esssans/src/ess/sans/__init__.py b/packages/esssans/src/ess/sans/__init__.py index c44ac4139..7713ccfa9 100644 --- a/packages/esssans/src/ess/sans/__init__.py +++ b/packages/esssans/src/ess/sans/__init__.py @@ -20,7 +20,6 @@ providers, with_background_runs, with_banks, - with_pixel_mask_filenames, with_sample_runs, ) @@ -49,6 +48,5 @@ 'providers', 'with_background_runs', 'with_banks', - 'with_pixel_mask_filenames', 'with_sample_runs', ] diff --git a/packages/esssans/src/ess/sans/io.py b/packages/esssans/src/ess/sans/io.py index 713825309..b95a0895b 100644 --- a/packages/esssans/src/ess/sans/io.py +++ b/packages/esssans/src/ess/sans/io.py @@ -7,10 +7,8 @@ from .types import ( BackgroundSubtractedIofQ, - MaskedDetectorIDs, Measurement, OutFilename, - PixelMaskFilename, SampleRun, ) @@ -33,43 +31,3 @@ def save_background_subtracted_iofq( title=measurement.title, run=measurement.run_number_maybe_int ) f['sasentry']['sasdata'] = nxcansas.SASdata(da, Q_variances='resolutions') - - -def read_xml_detector_masking(filename: PixelMaskFilename) -> MaskedDetectorIDs: - """Read a pixel mask from an ISIS XML file. - - The format is as follows, where the detids are inclusive ranges of detector IDs: - - .. code-block:: xml - - - - - 1400203-1400218,1401199,1402190-1402223 - - - - Parameters - ---------- - filename: - Path to the XML file. - """ - import xml.etree.ElementTree as ET # nosec - - tree = ET.parse(filename) # noqa: S314 - root = tree.getroot() - - masked_detids = [] - for group in root.findall('group'): - for detids in group.findall('detids'): - for detid in detids.text.split(','): - detid = detid.strip() - if '-' in detid: - start, stop = detid.split('-') - masked_detids += list(range(int(start), int(stop) + 1)) - else: - masked_detids.append(int(detid)) - - return MaskedDetectorIDs( - sc.array(dims=['detector_id'], values=masked_detids, unit=None, dtype='int32') - ) diff --git a/packages/esssans/src/ess/sans/masking.py b/packages/esssans/src/ess/sans/masking.py index 279f62273..b626240b7 100644 --- a/packages/esssans/src/ess/sans/masking.py +++ b/packages/esssans/src/ess/sans/masking.py @@ -4,17 +4,15 @@ Masking functions for the loki workflow. """ -import numpy as np -import scipp as sc +from ess.reduce.masking import read_pixel_masks, to_pixel_masks from .types import ( CorrectedDetector, DetectorIDs, DetectorMasks, EmptyDetector, - MaskedDetectorIDs, + LoadedPixelMasks, Numerator, - PixelMaskFilename, RunType, SampleRun, WavelengthDetector, @@ -30,26 +28,17 @@ def get_detector_ids_from_detector(data: EmptyDetector[SampleRun]) -> DetectorID ) -def to_detector_mask( - ids: DetectorIDs, - path: PixelMaskFilename, - masked_ids: MaskedDetectorIDs, -) -> DetectorMasks: - """Create a detector mask from a list of masked detector IDs. - The masks are based on detector IDs stored in XML files. +def to_detector_mask(ids: DetectorIDs, masks: LoadedPixelMasks) -> DetectorMasks: + """Create detector masks from the masks read from files. Parameters ---------- ids: Array of detector IDs - path: - Path to the mask file. - masked_ids: - Array of detector IDs which are masked. + masks: + Masks read from files. """ - mask = sc.zeros(sizes=ids.sizes, dtype='bool') - mask.values[np.isin(ids.values, masked_ids.values)] = True - return DetectorMasks({str(path): mask}) + return DetectorMasks(to_pixel_masks(masks, ids)) def apply_pixel_masks( @@ -70,6 +59,7 @@ def apply_pixel_masks( providers = ( get_detector_ids_from_detector, + read_pixel_masks, to_detector_mask, apply_pixel_masks, ) diff --git a/packages/esssans/src/ess/sans/parameters.py b/packages/esssans/src/ess/sans/parameters.py index cfb5f1efe..1fee6665e 100644 --- a/packages/esssans/src/ess/sans/parameters.py +++ b/packages/esssans/src/ess/sans/parameters.py @@ -36,7 +36,7 @@ NeXusDetectorName, NeXusMonitorName, Numerator, - PixelMaskFilename, + PixelMaskFilenames, PixelShapePath, QBins, QxBins, @@ -65,8 +65,8 @@ parameter_registry[TransformationPath] = StringParameter.from_type( TransformationPath, default='' ) -parameter_registry[PixelMaskFilename] = MultiFilenameParameter.from_type( - PixelMaskFilename +parameter_registry[PixelMaskFilenames] = MultiFilenameParameter.from_type( + PixelMaskFilenames ) parameter_registry[PixelShapePath] = StringParameter.from_type( PixelShapePath, default='' diff --git a/packages/esssans/src/ess/sans/types.py b/packages/esssans/src/ess/sans/types.py index 86a4f4ed9..063fe9e3a 100644 --- a/packages/esssans/src/ess/sans/types.py +++ b/packages/esssans/src/ess/sans/types.py @@ -12,6 +12,7 @@ import sciline import scipp as sc +from ess.reduce import masking as masking_t from ess.reduce.nexus import types as reduce_t from ess.reduce.uncertainty import UncertaintyBroadcastMode as _UncertaintyBroadcastMode from ess.reduce.unwrap import types as unwrap_t @@ -180,14 +181,12 @@ class TransmissionRun(Generic[ScatteringRunType]): """Filename of the output""" -PixelMaskFilename = NewType('PixelMaskFilename', str) +PixelMaskFilenames = masking_t.PixelMaskFilenames +LoadedPixelMasks = masking_t.LoadedPixelMasks DetectorIDs = NewType('DetectorIDs', sc.Variable) """1-D variable listing all detector IDs.""" -MaskedDetectorIDs = NewType('MaskedDetectorIDs', sc.Variable) -"""1-D variable listing all masked detector IDs.""" - DetectorMasks = NewType('DetectorMasks', dict[str, sc.Variable]) """Masks for detector pixels""" diff --git a/packages/esssans/src/ess/sans/workflow.py b/packages/esssans/src/ess/sans/workflow.py index fa854f8ad..b38fcadb8 100644 --- a/packages/esssans/src/ess/sans/workflow.py +++ b/packages/esssans/src/ess/sans/workflow.py @@ -15,7 +15,6 @@ CorrectForGravity, Denominator, DetectorBankSizes, - DetectorMasks, DimsToKeep, EmptyBeamRun, Filename, @@ -24,7 +23,6 @@ NormalizedQ, NormalizedQxQy, Numerator, - PixelMaskFilename, SampleRun, TransformationPath, Transmission, @@ -34,10 +32,6 @@ ) -def _merge(*dicts: dict) -> dict: - return {key: value for d in dicts for key, value in d.items()} - - def merge_contributions(*data: sc.DataArray) -> sc.DataArray: if len(data) == 1: return data[0] @@ -45,28 +39,6 @@ def merge_contributions(*data: sc.DataArray) -> sc.DataArray: return reducer.bins.concat() if data[0].bins is not None else reducer.sum() -def with_pixel_mask_filenames( - workflow: sciline.Pipeline, masks: Iterable[str] -) -> sciline.Pipeline: - """ - Return modified workflow with pixel mask filenames set. - - Parameters - ---------- - workflow: - Workflow to modify. - masks: - List or tuple of pixel mask filenames to set. - """ - workflow = workflow.copy() - workflow[DetectorMasks] = ( - workflow[DetectorMasks] - .map(pd.DataFrame({PixelMaskFilename: masks}).rename_axis('mask')) - .reduce(index='mask', func=_merge) - ) - return workflow - - def with_banks( workflow: sciline.Pipeline, banks: Iterable[str], @@ -141,7 +113,6 @@ def with_background_runs( return _set_runs(workflow, runs, BackgroundRun, 'background_run') -parameter_mappers[PixelMaskFilename] = with_pixel_mask_filenames # TODO: for now, we leave the mapping over detector banks out, because we do not have a # method to merge the I(Q) of different banks, and we thus cannot compute a single # result from the workflow. So only a single detector bank can be processed at a time. diff --git a/packages/esssans/tests/isissans/zoom_reduction_test.py b/packages/esssans/tests/isissans/zoom_reduction_test.py index b724b3b95..2867dac7c 100644 --- a/packages/esssans/tests/isissans/zoom_reduction_test.py +++ b/packages/esssans/tests/isissans/zoom_reduction_test.py @@ -16,6 +16,7 @@ IntensityQxQy, NeXusMonitorName, NonBackgroundWavelengthRange, + PixelMaskFilenames, QBins, QxBins, QyBins, @@ -69,9 +70,7 @@ def pipeline(): def test_can_create_pipeline(pipeline): pipeline[BeamCenter] = sc.vector([0, 0, 0], unit='m') - pipeline = sans.with_pixel_mask_filenames( - pipeline, isis.data.zoom_tutorial_mask_filenames() - ) + pipeline[PixelMaskFilenames] = isis.data.zoom_tutorial_mask_filenames() pipeline.get(IntensityQ[SampleRun]) @@ -83,18 +82,14 @@ def test_can_create_polarization_transmission_pipeline(): def test_pipeline_can_compute_IofQ(pipeline): pipeline[BeamCenter] = sc.vector([0, 0, 0], unit='m') - pipeline = sans.with_pixel_mask_filenames( - pipeline, isis.data.zoom_tutorial_mask_filenames() - ) + pipeline[PixelMaskFilenames] = isis.data.zoom_tutorial_mask_filenames() result = pipeline.compute(IntensityQ[SampleRun]) assert result.dims == ('Q',) def test_pipeline_can_compute_IofQxQy(pipeline): pipeline[BeamCenter] = sc.vector([0, 0, 0], unit='m') - pipeline = sans.with_pixel_mask_filenames( - pipeline, isis.data.zoom_tutorial_mask_filenames() - ) + pipeline[PixelMaskFilenames] = isis.data.zoom_tutorial_mask_filenames() pipeline[QxBins] = sc.linspace( dim='Qx', start=-0.5, stop=0.5, num=101, unit='1/angstrom' ) diff --git a/packages/esssans/tests/loki/iofq_test.py b/packages/esssans/tests/loki/iofq_test.py index 4d445b867..444f72344 100644 --- a/packages/esssans/tests/loki/iofq_test.py +++ b/packages/esssans/tests/loki/iofq_test.py @@ -24,6 +24,7 @@ IntensityQxQy, NormalizedQ, Numerator, + PixelMaskFilenames, QBins, QxBins, QyBins, @@ -44,9 +45,7 @@ def test_can_create_pipeline(larmor_workflow): def test_can_create_pipeline_with_pixel_masks(larmor_workflow): pipeline = larmor_workflow(no_masks=False) - pipeline = sans.with_pixel_mask_filenames( - pipeline, loki.data.loki_tutorial_mask_filenames() - ) + pipeline[PixelMaskFilenames] = loki.data.loki_tutorial_mask_filenames() pipeline[BeamCenter] = sc.vector([0, 0, 0], unit='m') pipeline.get(BackgroundSubtractedIofQ) @@ -59,9 +58,7 @@ def test_can_create_pipeline_with_pixel_masks(larmor_workflow): def test_pipeline_can_compute_IofQ(larmor_workflow, uncertainties, qxy: bool): pipeline = larmor_workflow(no_masks=False) pipeline[UncertaintyBroadcastMode] = uncertainties - pipeline = sans.with_pixel_mask_filenames( - pipeline, loki.data.loki_tutorial_mask_filenames() - ) + pipeline[PixelMaskFilenames] = loki.data.loki_tutorial_mask_filenames() pipeline[BeamCenter] = sans.beam_center_from_center_of_mass(pipeline) if qxy: result = pipeline.compute(BackgroundSubtractedIofQxy) @@ -278,9 +275,7 @@ def test_pipeline_IofQ_merging_events_yields_consistent_results(larmor_workflow) def test_beam_center_from_center_of_mass_is_close_to_verified_result(larmor_workflow): pipeline = larmor_workflow(no_masks=False) - pipeline = sans.with_pixel_mask_filenames( - pipeline, loki.data.loki_tutorial_mask_filenames() - ) + pipeline[PixelMaskFilenames] = loki.data.loki_tutorial_mask_filenames() center = sans.beam_center_from_center_of_mass(pipeline) reference = sc.vector([-0.0291487, -0.0181614, 4.1161], unit='m') assert sc.allclose(center, reference) @@ -290,9 +285,7 @@ def test_beam_center_from_center_of_mass_alternative_is_close_to_verified_result larmor_workflow, ): pipeline = larmor_workflow(no_masks=False) - pipeline = sans.with_pixel_mask_filenames( - pipeline, loki.data.loki_tutorial_mask_filenames() - ) + pipeline[PixelMaskFilenames] = loki.data.loki_tutorial_mask_filenames() center = sans.beam_center_finder.beam_center_from_center_of_mass_alternative( pipeline ) diff --git a/packages/esssans/tests/loki/workflow_test.py b/packages/esssans/tests/loki/workflow_test.py index 74d8e271b..47a86447c 100644 --- a/packages/esssans/tests/loki/workflow_test.py +++ b/packages/esssans/tests/loki/workflow_test.py @@ -18,7 +18,7 @@ IntensityQ, LookupTableFilename, NeXusDetectorName, - PixelMaskFilename, + PixelMaskFilenames, QBins, ReturnEvents, SampleRun, @@ -84,7 +84,7 @@ def test_solid_angle_is_independent_of_beam_center(larmor_workflow): def test_loki_larmor_workflow_compute_with_single_pixel_mask(larmor_workflow): wf = larmor_workflow(no_masks=False) wf[UncertaintyBroadcastMode] = UncertaintyBroadcastMode.drop - wf[PixelMaskFilename] = loki.data.loki_tutorial_mask_filenames()[0] + wf[PixelMaskFilenames] = loki.data.loki_tutorial_mask_filenames() # For simplicity, insert a fake beam center instead of computing it. wf[BeamCenter] = sc.vector([0.0, 0.0, 0.0], unit='m') From b043d427edb1ce0dba3fe4ffa77820fcefc3bdab Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Mon, 9 Nov 2026 10:57:33 +0000 Subject: [PATCH 02/16] Support NeXus pixel mask files with masks per detector bank A NeXus file can now hold a mask for each detector bank, in the logical dimensions of the bank. This allows small masks of tubes or straws (scipp/ess#767), which a list of detector IDs cannot express without listing every pixel. Masks use the pixel_mask field of NXdetector, with HDF5 dimension labels. Mask files are archived for years, so they do not use the scipp HDF5 layout, which is defined only by the code of scipp. The ADR records the layout and the reasons. Masks are checked when converted, because scipp silently ignores a mask with a dimension that the data lacks. A key that is not a bank of the instrument raises, since a misspelled name would otherwise mask nothing. The SANS provider now needs the bank name. Zoom has none, so ISIS gets a provider without it, and a per-bank file raises there. Co-Authored-By: Claude Opus 5.5 --- .../adr/0001-pixel-mask-file-format.md | 88 ++++++++ .../architecture-decision-records.md | 10 + packages/essreduce/docs/developer/index.md | 1 + packages/essreduce/src/ess/reduce/masking.py | 189 +++++++++++++++--- packages/essreduce/tests/masking_test.py | 155 ++++++++++++++ packages/esssans/src/ess/isissans/general.py | 15 ++ packages/esssans/src/ess/sans/masking.py | 17 +- packages/esssans/tests/loki/workflow_test.py | 40 ++++ 8 files changed, 487 insertions(+), 28 deletions(-) create mode 100644 packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md create mode 100644 packages/essreduce/docs/developer/architecture-decision-records.md diff --git a/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md b/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md new file mode 100644 index 000000000..8c15c16da --- /dev/null +++ b/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md @@ -0,0 +1,88 @@ +# ADR 0001: File format for pixel masks + +- Status: proposed +- Date: 2026-11 + +## Context + +Instrument scientists will create pixel masks, which are stored as files and used by reduction workflows for years. +Some are static, such as broken tubes, and are replaced when the hardware changes (see [#767](https://github.com/scipp/ess/issues/767)). +A file that exists in a data catalog cannot be changed by a software release, so this format is harder to change than the code that reads it. +The files must remain readable without ESS software or a particular version of it. + +The existing formats are ISIS XML and scipp HDF5 files holding a list of masked detector IDs. +Neither has a place for a bank, a name, or a validity date. +The planned masking tool for LoKI works on tubes and straws of a bank, after the detector data is folded into its logical dimensions. + +## Decision + +A mask file is a NeXus file, which is an HDF5 file with a defined structure: + +```text +/entry NXentry + title "broken-tubes", optional + valid_from "2026-11-01T00:00:00Z", not read by workflows + instrument NXinstrument + loki_detector_0 NXdetector + pixel_mask int32, HDF5 dimension labels ("layer", "tube") + loki_detector_1 NXdetector + pixel_mask ... +``` + +The content can be read with any HDF5 library: + +```python +with h5py.File(path) as f: + dataset = f['entry/instrument/loki_detector_0/pixel_mask'] + masked = (dataset[()] & 0xFFFF) != 0 + dims = [dim.label for dim in dataset.dims] +``` + +- Names of `NXdetector` groups are the names of the groups in the NeXus files of the runs. + A bank without `pixel_mask` has no mask from this file. + A name that is not a bank of the instrument is an error, since a misspelled name would otherwise mask nothing. +- `pixel_mask` is the integer bit field defined by NeXus for [`NXdetector`](https://manual.nexusformat.org/classes/base_classes/NXdetector.html). + A pixel is masked if any bit in `pixel_mask & 0xFFFF` is set, as NeXus specifies. + The other bits are tags, which do not mask a pixel. + `ess.reduce.masking.write_pixel_masks` sets bit 8, "user defined mask". +- Dimensions of `pixel_mask` are named with HDF5 dimension labels. + They are a subset of the logical dimensions of the bank, with matching sizes. + This is checked when masks are applied, because scipp ignores a mask with a dimension that the data lacks, without an error. +- Each file gives one mask per bank. + Tube and straw masks combine into one `(layer, tube, straw)` array. + The `pixel_mask_N` fields of NeXus are an error, so that they are not ignored. +- `title` becomes the name of the mask in the reduced data. + Without it, the name of the file is used. + Two masks with the same name are an error. + +Workflows take a list of filenames and read all `pixel_mask` fields. +They do not check validity dates. +Selecting the files that are valid for a run is left to the caller of the workflow, which sets the list of filenames. + +The readers for the existing formats stay, since they carry a list of detector IDs that works without a bank name. +New files should not use scipp HDF5. + +## Alternatives considered + +- **A scipp HDF5 file holding a `DataGroup`.** + This needs no writer, but the layout is defined by the code of scipp and not by a specification, so an archived file would depend on that code. +- **HDF5 with our own group names and a boolean dataset.** + It would need its own specification of what NeXus already defines. + HDF5 also has no boolean type: h5py stores booleans as an enumeration, which is a convention of h5py. +- **Lists of detector IDs, for new files too.** + `detector_number` is unique across the banks of LoKI, so such a file needs no bank level, and it stays correct if the layout of a bank changes. + It cannot express a mask of a tube or a straw except by listing every pixel. +- **Workflows check the validity date.** + This would require each workflow to know the time of a run and how to handle expired masks, while a list of filenames is enough for it to do its job. + +## Consequences + +- The format goes beyond the NeXus definition in three places. + NeXus describes `pixel_mask` with rank 2, while masks here have the rank of the logical dimensions they use. + Dimension labels are an HDF5 feature, not defined by NeXus. + `valid_from` is not a field that NeXus defines for `NXentry`. +- Boolean masks are positional. + A change of a bank's layout that keeps its shape would apply old masks to the wrong pixels without an error. + Mask files must be recreated when the layout of a bank changes. +- Mask files have no format version. + A later incompatible change needs a different location than `/entry/instrument//pixel_mask`. diff --git a/packages/essreduce/docs/developer/architecture-decision-records.md b/packages/essreduce/docs/developer/architecture-decision-records.md new file mode 100644 index 000000000..c26fa052c --- /dev/null +++ b/packages/essreduce/docs/developer/architecture-decision-records.md @@ -0,0 +1,10 @@ +# Architecture Decision Records + +```{toctree} +--- +maxdepth: 1 +glob: true +--- + +adr/* +``` diff --git a/packages/essreduce/docs/developer/index.md b/packages/essreduce/docs/developer/index.md index e47b24cf0..5b6424ea3 100644 --- a/packages/essreduce/docs/developer/index.md +++ b/packages/essreduce/docs/developer/index.md @@ -14,4 +14,5 @@ getting-started coding-conventions dependency-management gui +architecture-decision-records ``` diff --git a/packages/essreduce/src/ess/reduce/masking.py b/packages/essreduce/src/ess/reduce/masking.py index e58afd0e2..4e4186708 100644 --- a/packages/essreduce/src/ess/reduce/masking.py +++ b/packages/essreduce/src/ess/reduce/masking.py @@ -6,30 +6,67 @@ once. Converting the file content to masks for a detector is a separate step. """ -from collections.abc import Sequence +import re +from collections.abc import Collection, Mapping, Sequence from pathlib import Path from typing import NewType import numpy as np import scipp as sc +import scippnexus as snx PixelMaskFilenames = NewType('PixelMaskFilenames', Sequence[str | Path]) """Files with pixel masks to apply. Use an empty sequence to apply none.""" -LoadedPixelMasks = NewType('LoadedPixelMasks', dict[str, sc.Variable]) -"""Masked detector IDs from each mask file, by mask name.""" +LoadedPixelMasks = NewType('LoadedPixelMasks', dict[str, sc.Variable | sc.DataGroup]) +"""Content of each mask file, by mask name. + +Either a variable of masked detector IDs, or a data group of boolean masks by detector +bank. +""" + +_HDF5_SUFFIXES = ('.h5', '.hdf', '.hdf5', '.nxs') + +_USER_DEFINED_MASK = 1 << 8 +"""Bit of the NeXus ``pixel_mask`` field for a mask defined by the user.""" + +_REJECTING_BITS = 0xFFFF +"""Bits of the NeXus ``pixel_mask`` field that exclude a pixel from analysis. + +Other bits are tags, which do not exclude a pixel on their own. +""" def read_pixel_masks(filenames: PixelMaskFilenames) -> LoadedPixelMasks: """Read pixel masks from files. - A mask is named after its file, without the directory, since directories differ - between hosts. The following formats are supported, selected by file suffix: + The following formats are supported, selected by file suffix and content: - ``.xml``: Masked detector IDs in the ISIS format, see :py:func:`read_xml_masked_detector_ids`. - - ``.h5``, ``.hdf``, ``.hdf5``: A scipp variable of masked detector IDs, as written - by :py:func:`scipp.io.save_hdf5`. + - ``.h5``, ``.hdf``, ``.hdf5``, ``.nxs``: Either of + + - A NeXus file with masks by detector bank, see :py:func:`write_pixel_masks`: + + .. code-block:: text + + /entry (NXentry) + title name of the mask, optional + instrument (NXinstrument) + loki_detector_0 (NXdetector) + pixel_mask integer array + + Bank names are the names of the NeXus detector groups. A bank without + ``pixel_mask`` has no mask from this file. Following NeXus, a pixel is masked + if any bit in ``pixel_mask & 0xFFFF`` is set. The dimensions of ``pixel_mask`` + are given by HDF5 dimension labels. They are named after the logical dimensions + of the bank, and may be a subset of them. Other entries of the file, such as a + validity date, are ignored. + - A file written by :py:func:`scipp.io.save_hdf5` with a variable of masked + detector IDs. + + A mask has the name stored in its file. Otherwise it is named after its file, + without the directory, since directories differ between hosts. Parameters ---------- @@ -44,28 +81,85 @@ def read_pixel_masks(filenames: PixelMaskFilenames) -> LoadedPixelMasks: """ masks = {} for filename in map(Path, filenames): - if filename.name in masks: - raise ValueError(f"Several mask files have the name '{filename.name}'.") - masks[filename.name] = _read_mask_file(filename) + name, content = _read_mask_file(filename) + if name in masks: + raise ValueError(f"Several mask files give the mask name '{name}'.") + masks[name] = content return LoadedPixelMasks(masks) def to_pixel_masks( - loaded: LoadedPixelMasks, ids: sc.Variable + loaded: LoadedPixelMasks, + ids: sc.Variable, + bank: str | None = None, + known_banks: Collection[str] | None = None, ) -> dict[str, sc.Variable]: - """Create a boolean mask per loaded mask, for the pixels of a detector. + """Create boolean masks for the pixels of a detector bank. + + Masks given as detector IDs apply to any bank, since IDs identify a pixel. They + have the dimensions of ``ids``. Masks given per bank are selected by ``bank``. Parameters ---------- loaded: Masks read from files. ids: - Detector IDs of the pixels. The masks have the dimensions of this variable. + Detector IDs of the pixels. + bank: + Name of the detector bank. Without it, masks given per bank raise an error. + known_banks: + Names of all banks of the instrument. If given, a file with masks for any other + bank raises an error. A misspelled name would otherwise mask nothing. """ - return { - name: sc.array(dims=ids.dims, values=np.isin(ids.values, masked.values)) - for name, masked in loaded.items() - } + masks = {} + for name, content in loaded.items(): + if isinstance(content, sc.Variable): + masks[name] = sc.array( + dims=ids.dims, values=np.isin(ids.values, content.values) + ) + continue + if bank is None: + raise ValueError(f"Mask '{name}' is given per bank, but no bank is known.") + if known_banks is not None and (unknown := set(content) - set(known_banks)): + raise ValueError(f"Mask '{name}' has unknown banks: {sorted(unknown)}.") + if bank in content: + masks[name] = _validate_bank_mask(content[bank], ids, name) + return masks + + +def write_pixel_masks( + filename: str | Path, + masks: Mapping[str, sc.Variable], + name: str | None = None, +) -> None: + """Write boolean masks by detector bank to a NeXus file. + + See :py:func:`read_pixel_masks` for the layout of the file. + + Parameters + ---------- + filename: + Path of the file to write. + masks: + Boolean mask of each detector bank. Dimensions are named after the logical + dimensions of the bank, and may be a subset of them. + name: + Name of the mask. Without it, the mask is named after the file when it is read. + """ + with snx.File(filename, 'w') as file: + entry = file.create_class('entry', snx.NXentry) + if name is not None: + entry.create_field('title', name) + instrument = entry.create_class('instrument', snx.NXinstrument) + for bank, mask in masks.items(): + detector = instrument.create_class(bank, snx.NXdetector) + bits = np.where(mask.values, _USER_DEFINED_MASK, 0) + dataset = detector.create_field( + 'pixel_mask', + sc.array(dims=mask.dims, values=bits, unit=None, dtype='int32'), + ) + for axis, dim in zip(dataset.dims, mask.dims, strict=True): + axis.label = dim def read_xml_masked_detector_ids(filename: str | Path) -> sc.Variable: @@ -108,15 +202,60 @@ def read_xml_masked_detector_ids(filename: str | Path) -> sc.Variable: ) -def _read_mask_file(filename: Path) -> sc.Variable: +def _read_mask_file(filename: Path) -> tuple[str, sc.Variable | sc.DataGroup]: if filename.suffix == '.xml': - return read_xml_masked_detector_ids(filename) - if filename.suffix in ('.h5', '.hdf', '.hdf5'): - return _validate_ids(sc.io.load_hdf5(filename), filename) + return filename.name, read_xml_masked_detector_ids(filename) + if filename.suffix in _HDF5_SUFFIXES: + return _read_hdf5(filename) raise ValueError(f"Unsupported suffix of mask file '{filename}'.") -def _validate_ids(content: object, filename: Path) -> sc.Variable: - if isinstance(content, sc.Variable) and content.dtype in ('int32', 'int64'): - return content - raise ValueError(f"Mask file '{filename}' does not hold integer detector IDs.") +def _read_hdf5(filename: Path) -> tuple[str, sc.Variable | sc.DataGroup]: + with snx.File(filename) as file: + if 'entry' in file: + return _read_nexus_masks(file['entry'], filename) + ids = sc.io.load_hdf5(filename) + if isinstance(ids, sc.Variable) and ids.dtype in ('int32', 'int64'): + return filename.name, ids + raise ValueError( + f"Mask file '{filename}' holds neither a NeXus entry nor integer detector IDs." + ) + + +def _read_nexus_masks(entry: snx.Group, filename: Path) -> tuple[str, sc.DataGroup]: + masks = { + bank: _read_bank_mask(detector, filename) + for bank, detector in entry['instrument'][snx.NXdetector].items() + if 'pixel_mask' in detector + } + if not masks: + raise ValueError(f"Mask file '{filename}' has no detector with a pixel_mask.") + name = entry['title'][()] if 'title' in entry else filename.name + return name, sc.DataGroup(masks) + + +def _read_bank_mask(detector: snx.Group, filename: Path) -> sc.Variable: + # NeXus combines these into one mask. They are rejected instead of ignored, since + # ignoring them would leave pixels unmasked. + if numbered := [key for key in detector if re.fullmatch(r'pixel_mask_\d+', key)]: + raise ValueError( + f"Mask file '{filename}' has several masks for one bank: {numbered}." + ) + field = detector['pixel_mask'] + # Loading the field as a variable would clip uint32 to int32 and change the bits. + values = field.dataset[()] + if not np.issubdtype(values.dtype, np.integer): + raise ValueError(f"Mask file '{filename}' has a non-integer pixel_mask.") + # int64 holds all bits to test for any input type, unlike e.g. uint8. + masked = values.astype('int64') & _REJECTING_BITS != 0 + return sc.array(dims=field.dims, values=masked) + + +def _validate_bank_mask(mask: sc.Variable, ids: sc.Variable, name: str) -> sc.Variable: + # Masks with a dimension that the data lacks would be ignored without an error. + if not mask.sizes.items() <= ids.sizes.items(): + raise sc.DimensionError( + f"Mask '{name}' has sizes {mask.sizes}, which are not part of the sizes " + f"of the detector, {ids.sizes}." + ) + return mask diff --git a/packages/essreduce/tests/masking_test.py b/packages/essreduce/tests/masking_test.py index 2e7bc3ac2..c76a4aa8a 100644 --- a/packages/essreduce/tests/masking_test.py +++ b/packages/essreduce/tests/masking_test.py @@ -2,14 +2,17 @@ # Copyright (c) 2026 Scipp contributors (https://github.com/scipp) from pathlib import Path +import numpy as np import pytest import scipp as sc +import scippnexus as snx from ess.reduce.masking import ( LoadedPixelMasks, PixelMaskFilenames, read_pixel_masks, to_pixel_masks, + write_pixel_masks, ) @@ -32,6 +35,16 @@ def _read(*paths: Path) -> LoadedPixelMasks: return read_pixel_masks(PixelMaskFilenames(paths)) +def _write_raw_pixel_mask(path: Path, field: str, values: np.ndarray) -> Path: + with snx.File(path, 'w') as file: + entry = file.create_class('entry', snx.NXentry) + instrument = entry.create_class('instrument', snx.NXinstrument) + detector = instrument.create_class('bank_0', snx.NXdetector) + dataset = detector.create_field(field, values) + dataset.dims[0].label = 'tube' + return path + + def test_read_no_files_gives_no_masks() -> None: assert read_pixel_masks(PixelMaskFilenames([])) == {} @@ -76,9 +89,151 @@ def test_read_raises_if_hdf5_does_not_hold_integers(tmp_path: Path) -> None: _read(path) +def test_read_bank_masks_names_mask_after_file_by_default(tmp_path: Path) -> None: + mask = sc.array(dims=['tube'], values=[True, False]) + write_pixel_masks(tmp_path / 'a.nxs', {'bank_0': mask}) + masks = _read(tmp_path / 'a.nxs') + assert sc.identical(masks['a.nxs']['bank_0'], mask) + + +def test_read_bank_masks_uses_name_stored_in_file(tmp_path: Path) -> None: + mask = sc.array(dims=['tube'], values=[True, False]) + write_pixel_masks(tmp_path / 'a.nxs', {'bank_0': mask}, name='broken') + assert list(_read(tmp_path / 'a.nxs')) == ['broken'] + + +def test_read_bank_masks_keeps_dimensions_of_all_banks(tmp_path: Path) -> None: + banks = { + 'bank_0': sc.array(dims=['layer', 'tube'], values=[[True, False]]), + 'bank_1': sc.array(dims=['straw'], values=[False, True, False]), + } + write_pixel_masks(tmp_path / 'a.nxs', banks) + loaded = _read(tmp_path / 'a.nxs')['a.nxs'] + assert sc.identical(loaded, sc.DataGroup(banks)) + + +def test_read_ignores_other_entries_of_the_file(tmp_path: Path) -> None: + path = tmp_path / 'a.nxs' + write_pixel_masks(path, {'bank_0': sc.array(dims=['tube'], values=[True])}) + with snx.File(path, 'r+') as file: + file['entry'].create_field('valid_from', '2026-11-01T00:00:00Z') + assert list(_read(path)['a.nxs']) == ['bank_0'] + + +def test_read_skips_detector_without_pixel_mask(tmp_path: Path) -> None: + path = tmp_path / 'a.nxs' + write_pixel_masks(path, {'bank_0': sc.array(dims=['tube'], values=[True])}) + with snx.File(path, 'r+') as file: + file['entry/instrument'].create_class('bank_1', snx.NXdetector) + assert list(_read(path)['a.nxs']) == ['bank_0'] + + +def test_read_raises_if_stored_names_are_equal(tmp_path: Path) -> None: + mask = sc.array(dims=['tube'], values=[True]) + write_pixel_masks(tmp_path / 'a.nxs', {'bank_0': mask}, name='broken') + write_pixel_masks(tmp_path / 'b.nxs', {'bank_0': mask}, name='broken') + with pytest.raises(ValueError, match='broken'): + _read(tmp_path / 'a.nxs', tmp_path / 'b.nxs') + + +@pytest.mark.parametrize('dtype', ['uint8', 'int32', 'int64']) +def test_read_masks_pixels_with_any_rejecting_bit(tmp_path: Path, dtype: str) -> None: + values = np.array([0, 1, 2], dtype=dtype) + path = _write_raw_pixel_mask(tmp_path / 'a.nxs', 'pixel_mask', values) + loaded = _read(path)['a.nxs']['bank_0'] + assert loaded.dims == ('tube',) + assert loaded.values.tolist() == [False, True, True] + + +def test_read_does_not_mask_pixels_with_tag_bits_only(tmp_path: Path) -> None: + values = np.array([1 << 8, 1 << 31], dtype='uint32') + path = _write_raw_pixel_mask(tmp_path / 'a.nxs', 'pixel_mask', values) + assert _read(path)['a.nxs']['bank_0'].values.tolist() == [True, False] + + +def test_read_raises_if_pixel_mask_is_not_integer(tmp_path: Path) -> None: + path = _write_raw_pixel_mask(tmp_path / 'a.nxs', 'pixel_mask', np.array([1.5])) + with pytest.raises(ValueError, match='non-integer'): + _read(path) + + +def test_read_raises_on_numbered_pixel_masks(tmp_path: Path) -> None: + path = _write_raw_pixel_mask(tmp_path / 'a.nxs', 'pixel_mask', np.array([1])) + with snx.File(path, 'r+') as file: + file['entry/instrument/bank_0'].create_field('pixel_mask_2', np.array([1])) + with pytest.raises(ValueError, match='pixel_mask_2'): + _read(path) + + +def test_read_raises_if_file_has_no_pixel_mask(tmp_path: Path) -> None: + path = _write_raw_pixel_mask(tmp_path / 'a.nxs', 'detector_number', np.array([1])) + with pytest.raises(ValueError, match='no detector with a pixel_mask'): + _read(path) + + def test_to_pixel_masks_keeps_shape_of_ids() -> None: ids = sc.array(dims=['tube', 'pixel'], values=[[1, 2], [3, 4]], unit=None) masked = sc.array(dims=['detector_id'], values=[2, 3, 99], unit=None) masks = to_pixel_masks(LoadedPixelMasks({'m': masked}), ids) expected = sc.array(dims=['tube', 'pixel'], values=[[False, True], [True, False]]) assert sc.identical(masks['m'], expected) + + +@pytest.fixture +def bank_ids() -> sc.Variable: + return sc.array(dims=['tube', 'pixel'], values=[[1, 2], [3, 4], [5, 6]], unit=None) + + +def _per_bank(**banks: sc.Variable) -> LoadedPixelMasks: + return LoadedPixelMasks({'m': sc.DataGroup(banks)}) + + +def test_to_pixel_masks_selects_bank_and_allows_fewer_dims( + bank_ids: sc.Variable, +) -> None: + mask = sc.array(dims=['tube'], values=[True, False, False]) + other = sc.array(dims=['tube'], values=[False, True, False]) + masks = to_pixel_masks(_per_bank(a=mask, b=other), bank_ids, bank='a') + assert sc.identical(masks['m'], mask) + + +def test_to_pixel_masks_skips_file_without_this_bank(bank_ids: sc.Variable) -> None: + mask = sc.array(dims=['tube'], values=[True, False, False]) + assert to_pixel_masks(_per_bank(b=mask), bank_ids, bank='a') == {} + + +def test_to_pixel_masks_applies_ids_to_any_bank(bank_ids: sc.Variable) -> None: + masked = sc.array(dims=['detector_id'], values=[1], unit=None) + masks = to_pixel_masks(LoadedPixelMasks({'m': masked}), bank_ids, bank='a') + assert masks['m'].values.sum() == 1 + + +def test_to_pixel_masks_raises_without_bank_for_masks_per_bank( + bank_ids: sc.Variable, +) -> None: + mask = sc.array(dims=['tube'], values=[True, False, False]) + with pytest.raises(ValueError, match='no bank is known'): + to_pixel_masks(_per_bank(a=mask), bank_ids) + + +def test_to_pixel_masks_raises_on_unknown_bank(bank_ids: sc.Variable) -> None: + mask = sc.array(dims=['tube'], values=[True, False, False]) + with pytest.raises(ValueError, match='typo'): + to_pixel_masks( + _per_bank(a=mask, typo=mask), bank_ids, bank='a', known_banks={'a', 'b'} + ) + + +@pytest.mark.parametrize( + 'mask', + [ + sc.array(dims=['straw'], values=[True, False, False]), + sc.array(dims=['tube'], values=[True, False]), + ], + ids=['unknown-dim', 'wrong-size'], +) +def test_to_pixel_masks_raises_if_mask_does_not_fit_detector( + bank_ids: sc.Variable, mask: sc.Variable +) -> None: + with pytest.raises(sc.DimensionError): + to_pixel_masks(_per_bank(a=mask), bank_ids, bank='a') diff --git a/packages/esssans/src/ess/isissans/general.py b/packages/esssans/src/ess/isissans/general.py index b8d4968c6..418e3b2b3 100644 --- a/packages/esssans/src/ess/isissans/general.py +++ b/packages/esssans/src/ess/isissans/general.py @@ -13,11 +13,13 @@ import scippnexus as snx from ess.sans.types import ( DetectorIDs, + DetectorMasks, DetectorPixelShape, DetectorPositionOffset, EmptyDetector, EmptyMonitor, Incident, + LoadedPixelMasks, Measurement, MonitorPositionOffset, MonitorType, @@ -33,6 +35,7 @@ WavelengthMonitor, ) +from ess.reduce.masking import to_pixel_masks from ess.reduce.nexus.types import NeXusTransformation, Position from .io import LoadedFileContents @@ -301,6 +304,17 @@ def get_detector_ids_from_sample_run(data: RawDetector[SampleRun]) -> DetectorID ) +def to_detector_mask_without_bank( + ids: DetectorIDs, masks: LoadedPixelMasks +) -> DetectorMasks: + """Create detector masks from masks read from files. + + This overrides the function in the masking module, since files loaded using Mantid + have no NeXus detector name. So mask files that give masks per bank are not usable. + """ + return DetectorMasks(to_pixel_masks(masks, ids)) + + providers = ( dummy_assemble_detector_data, dummy_assemble_monitor_data, @@ -313,6 +327,7 @@ def get_detector_ids_from_sample_run(data: RawDetector[SampleRun]) -> DetectorID get_calibrated_isis_detector, get_calibrated_isis_monitor, get_detector_ids_from_sample_run, + to_detector_mask_without_bank, get_monitor_data, data_to_wavelength, monitor_to_wavelength, diff --git a/packages/esssans/src/ess/sans/masking.py b/packages/esssans/src/ess/sans/masking.py index b626240b7..ecce79c00 100644 --- a/packages/esssans/src/ess/sans/masking.py +++ b/packages/esssans/src/ess/sans/masking.py @@ -8,10 +8,12 @@ from .types import ( CorrectedDetector, + DetectorBankSizes, DetectorIDs, DetectorMasks, EmptyDetector, LoadedPixelMasks, + NeXusDetectorName, Numerator, RunType, SampleRun, @@ -28,8 +30,13 @@ def get_detector_ids_from_detector(data: EmptyDetector[SampleRun]) -> DetectorID ) -def to_detector_mask(ids: DetectorIDs, masks: LoadedPixelMasks) -> DetectorMasks: - """Create detector masks from the masks read from files. +def to_detector_mask( + ids: DetectorIDs, + masks: LoadedPixelMasks, + bank: NeXusDetectorName, + bank_sizes: DetectorBankSizes, +) -> DetectorMasks: + """Create detector masks for a bank from the masks read from files. Parameters ---------- @@ -37,8 +44,12 @@ def to_detector_mask(ids: DetectorIDs, masks: LoadedPixelMasks) -> DetectorMasks Array of detector IDs masks: Masks read from files. + bank: + Name of the detector bank. + bank_sizes: + Sizes of the banks of the instrument, which define the valid bank names. """ - return DetectorMasks(to_pixel_masks(masks, ids)) + return DetectorMasks(to_pixel_masks(masks, ids, bank=bank, known_banks=bank_sizes)) def apply_pixel_masks( diff --git a/packages/esssans/tests/loki/workflow_test.py b/packages/esssans/tests/loki/workflow_test.py index 47a86447c..4bc0b8e2e 100644 --- a/packages/esssans/tests/loki/workflow_test.py +++ b/packages/esssans/tests/loki/workflow_test.py @@ -13,6 +13,8 @@ BackgroundRun, BackgroundSubtractedIofQ, BeamCenter, + DetectorIDs, + DetectorMasks, EmptyDetector, Filename, IntensityQ, @@ -29,6 +31,7 @@ from sciline import UnsatisfiedRequirement from ess.reduce import workflow +from ess.reduce.masking import write_pixel_masks from ess.reduce.nexus.types import DiskChoppers, Position from ess.reduce.unwrap import LookupTable, LtotalRange, SourceBounds @@ -154,3 +157,40 @@ def test_loki_analytical_lookup_table_matches_simulated_source_pulse(): value = row['event_time_offset', np.argmin(np.abs(toa - t))].data assert abs(value.value - mean) < 0.01 assert abs(sc.stddevs(value).value / stddev - 1) < 0.05 + + +def test_loki_workflow_reads_bool_masks_per_bank(tmp_path): + path = tmp_path / 'masks.h5' + mask = sc.array(dims=['tube'], values=[True, False]) + write_pixel_masks(path, {'loki_detector_1': mask}, name='broken') + wf = loki.LokiWorkflow() + wf[NeXusDetectorName] = 'loki_detector_1' + wf[DetectorIDs] = sc.array(dims=['tube'], values=[1, 2], unit=None) + wf[PixelMaskFilenames] = [path] + + assert sc.identical(wf.compute(DetectorMasks)['broken'], mask) + + +def test_loki_workflow_raises_if_mask_file_has_unknown_bank(tmp_path): + path = tmp_path / 'masks.h5' + mask = sc.array(dims=['tube'], values=[True, False]) + write_pixel_masks(path, {'loki_detector_99': mask}) + wf = loki.LokiWorkflow() + wf[NeXusDetectorName] = 'loki_detector_1' + wf[DetectorIDs] = sc.array(dims=['tube'], values=[1, 2], unit=None) + wf[PixelMaskFilenames] = [path] + + with pytest.raises(ValueError, match='loki_detector_99'): + wf.compute(DetectorMasks) + + +def test_bool_mask_file_gives_same_masks_as_id_file(larmor_workflow, tmp_path): + wf = larmor_workflow(no_masks=False) + wf[PixelMaskFilenames] = loki.data.loki_tutorial_mask_filenames() + from_ids = next(iter(wf.compute(DetectorMasks).values())) + + path = tmp_path / 'masks.h5' + write_pixel_masks(path, {'larmor_detector': from_ids}) + wf[PixelMaskFilenames] = [path] + + assert sc.identical(wf.compute(DetectorMasks)['masks.h5'], from_ids) From 4970eb16cb19fb6516941629526f2eb1a1228de6 Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Mon, 9 Nov 2026 12:47:20 +0000 Subject: [PATCH 03/16] Rename LoadedPixelMasks to PixelMasks The value does not have to come from a file: it can be set directly, as the BEER workflow does. It holds the pixel masks of the whole instrument, and DetectorMasks holds them for one detector. to_pixel_masks is renamed to to_detector_masks, since it returns what the SANS workflows call DetectorMasks. Co-Authored-By: Claude Opus 5.5 --- .../beer/beer_powder_mcstas_analytical.ipynb | 6 ++--- .../essdiffraction/src/ess/powder/masking.py | 8 +++--- .../essdiffraction/src/ess/powder/types.py | 2 +- .../essdiffraction/src/ess/powder/workflow.py | 4 +-- .../tests/beer/mcstas_reduction_test.py | 4 +-- .../tools/docs/beer-thumbnails.ipynb | 26 ++++++------------- packages/essreduce/src/ess/reduce/masking.py | 23 ++++++++-------- packages/essreduce/tests/masking_test.py | 24 ++++++++--------- packages/esssans/src/ess/isissans/general.py | 12 ++++----- packages/esssans/src/ess/sans/masking.py | 14 +++++----- packages/esssans/src/ess/sans/types.py | 2 +- 11 files changed, 57 insertions(+), 68 deletions(-) diff --git a/packages/essdiffraction/docs/user-guide/beer/beer_powder_mcstas_analytical.ipynb b/packages/essdiffraction/docs/user-guide/beer/beer_powder_mcstas_analytical.ipynb index 50ab9cbd3..d5efead2d 100644 --- a/packages/essdiffraction/docs/user-guide/beer/beer_powder_mcstas_analytical.ipynb +++ b/packages/essdiffraction/docs/user-guide/beer/beer_powder_mcstas_analytical.ipynb @@ -107,7 +107,7 @@ "workflow[KeepEvents[VanadiumRun]] = KeepEvents(True)\n", "workflow[KeepEvents[EmptyCanRun]] = KeepEvents(True)\n", "\n", - "workflow[LoadedPixelMasks] = LoadedPixelMasks({})\n", + "workflow[PixelMasks] = PixelMasks({})\n", "workflow[UncertaintyBroadcastMode] = UncertaintyBroadcastMode.drop" ] }, @@ -260,9 +260,7 @@ "unfiltered_detector = workflow.compute(CorrectedDetector[SampleRun])\n", "filtered_detector = thresholded_workflow.compute(CorrectedDetector[SampleRun])\n", "\n", - "two_theta_bins = sc.linspace(\"two_theta\", 76.0, 104.0, 201, unit=\"deg\").to(\n", - " unit=\"rad\"\n", - ")\n", + "two_theta_bins = sc.linspace(\"two_theta\", 76.0, 104.0, 201, unit=\"deg\").to(unit=\"rad\")\n", "wavelength_bins = sc.linspace(\"wavelength\", 0.8, 3.2, 241, unit=\"angstrom\")\n", "\n", "unfiltered_map = unfiltered_detector.hist(\n", diff --git a/packages/essdiffraction/src/ess/powder/masking.py b/packages/essdiffraction/src/ess/powder/masking.py index e10899ac8..06654212e 100644 --- a/packages/essdiffraction/src/ess/powder/masking.py +++ b/packages/essdiffraction/src/ess/powder/masking.py @@ -6,10 +6,10 @@ import scipp as sc -from ess.reduce.masking import read_pixel_masks, to_pixel_masks +from ess.reduce.masking import read_pixel_masks, to_detector_masks from .types import ( - LoadedPixelMasks, + PixelMasks, RunType, TofMask, TwoThetaMask, @@ -20,7 +20,7 @@ def apply_masks( data: WavelengthDetector[RunType], - pixel_masks: LoadedPixelMasks, + pixel_masks: PixelMasks, tof_mask_func: TofMask, wavelength_mask_func: WavelengthMask, two_theta_mask_func: TwoThetaMask, @@ -31,7 +31,7 @@ def apply_masks( key = ( set(out.coords.keys()) & {"detector_number", "detector_id", "spectrum"} ).pop() - out = out.assign_masks(to_pixel_masks(pixel_masks, out.coords[key])) + out = out.assign_masks(to_detector_masks(pixel_masks, out.coords[key])) for dim, mask in { "tof": tof_mask_func, diff --git a/packages/essdiffraction/src/ess/powder/types.py b/packages/essdiffraction/src/ess/powder/types.py index 86f96ade4..3b845eb6a 100644 --- a/packages/essdiffraction/src/ess/powder/types.py +++ b/packages/essdiffraction/src/ess/powder/types.py @@ -203,7 +203,7 @@ class NormalizedDspacing(sciline.Scope[RunType, sc.DataArray], sc.DataArray): PixelMaskFilenames = masking_t.PixelMaskFilenames -LoadedPixelMasks = masking_t.LoadedPixelMasks +PixelMasks = masking_t.PixelMasks class RawDataAndMetadata(sciline.Scope[RunType, sc.DataGroup], sc.DataGroup): diff --git a/packages/essdiffraction/src/ess/powder/workflow.py b/packages/essdiffraction/src/ess/powder/workflow.py index 3e71ad462..2407e001b 100644 --- a/packages/essdiffraction/src/ess/powder/workflow.py +++ b/packages/essdiffraction/src/ess/powder/workflow.py @@ -3,7 +3,7 @@ from .types import ( CorrectedDetector, DspacingDetector, - LoadedPixelMasks, + PixelMasks, RunType, TofMask, TwoThetaMask, @@ -13,7 +13,7 @@ def add_masks_and_corrections( da: DspacingDetector[RunType], - pixel_masks: LoadedPixelMasks, + pixel_masks: PixelMasks, tof_mask_func: TofMask, wavelength_mask_func: WavelengthMask, two_theta_mask_func: TwoThetaMask, diff --git a/packages/essdiffraction/tests/beer/mcstas_reduction_test.py b/packages/essdiffraction/tests/beer/mcstas_reduction_test.py index 4060e0f13..278294c37 100644 --- a/packages/essdiffraction/tests/beer/mcstas_reduction_test.py +++ b/packages/essdiffraction/tests/beer/mcstas_reduction_test.py @@ -49,8 +49,8 @@ ElasticCoordTransformGraph, EmptyDetector, KeepEvents, - LoadedPixelMasks, NormalizedDspacing, + PixelMasks, QDetector, SampleRun, TofMask, @@ -155,7 +155,7 @@ def test_modulation_workflows_can_normalize(factory): wf[DetectorBank] = DetectorBank.north wf[DHKLList] = silicon_peaks_array() wf[DspacingBins] = sc.linspace('dspacing', 0.8, 2.2, 31, unit='angstrom') - wf[LoadedPixelMasks] = LoadedPixelMasks({}) + wf[PixelMasks] = PixelMasks({}) wf[KeepEvents[SampleRun]] = KeepEvents[SampleRun](True) wf[UncertaintyBroadcastMode] = UncertaintyBroadcastMode.drop wf[TofMask] = None diff --git a/packages/essdiffraction/tools/docs/beer-thumbnails.ipynb b/packages/essdiffraction/tools/docs/beer-thumbnails.ipynb index 8060245ed..986260b33 100644 --- a/packages/essdiffraction/tools/docs/beer-thumbnails.ipynb +++ b/packages/essdiffraction/tools/docs/beer-thumbnails.ipynb @@ -31,7 +31,7 @@ " EmptyCanSubtractedIofDspacing,\n", " Filename,\n", " KeepEvents,\n", - " LoadedPixelMasks,\n", + " PixelMasks,\n", " SampleRun,\n", " TofMask,\n", " TwoThetaMask,\n", @@ -39,7 +39,7 @@ " VanadiumRun,\n", " WavelengthDetector,\n", " WavelengthMask,\n", - ")\n" + ")" ] }, { @@ -59,9 +59,7 @@ "modulation_data = modulation_result[WavelengthDetector[SampleRun]].transform_coords(\n", " 'two_theta', graph=modulation_result[ElasticCoordTransformGraph[SampleRun]]\n", ")\n", - "modulation_histogram = modulation_data.hist(\n", - " two_theta=400, event_time_offset=1000\n", - ")" + "modulation_histogram = modulation_data.hist(two_theta=400, event_time_offset=1000)" ] }, { @@ -137,23 +135,17 @@ "metadata": {}, "outputs": [], "source": [ - "powder_workflow = BeerPowderMcStasWorkflow(\n", - " run_norm=RunNormalization.monitor_histogram\n", - ")\n", - "powder_workflow[Filename[SampleRun]] = (\n", - " beer_data.mcstas_powder_silicon_in_vanadium_can()\n", - ")\n", + "powder_workflow = BeerPowderMcStasWorkflow(run_norm=RunNormalization.monitor_histogram)\n", + "powder_workflow[Filename[SampleRun]] = beer_data.mcstas_powder_silicon_in_vanadium_can()\n", "powder_workflow[Filename[VanadiumRun]] = beer_data.mcstas_powder_vanadium()\n", "powder_workflow[Filename[EmptyCanRun]] = beer_data.mcstas_powder_empty_can()\n", "powder_workflow[DetectorBank] = DetectorBank.north\n", - "powder_workflow[DspacingBins] = sc.linspace(\n", - " 'dspacing', 0.6, 2.0, 701, unit='angstrom'\n", - ")\n", + "powder_workflow[DspacingBins] = sc.linspace('dspacing', 0.6, 2.0, 701, unit='angstrom')\n", "\n", "for run_type in (SampleRun, VanadiumRun, EmptyCanRun):\n", " powder_workflow[KeepEvents[run_type]] = KeepEvents(True)\n", "\n", - "powder_workflow[LoadedPixelMasks] = LoadedPixelMasks({})\n", + "powder_workflow[PixelMasks] = PixelMasks({})\n", "powder_workflow[UncertaintyBroadcastMode] = UncertaintyBroadcastMode.drop\n", "powder_workflow[TwoThetaMask] = None\n", "powder_workflow[TofMask] = None\n", @@ -167,9 +159,7 @@ "metadata": {}, "outputs": [], "source": [ - "powder_pattern = powder_workflow.compute(\n", - " EmptyCanSubtractedIofDspacing\n", - ").hist()\n", + "powder_pattern = powder_workflow.compute(EmptyCanSubtractedIofDspacing).hist()\n", "powder_pattern = powder_pattern.assign(\n", " sc.where(\n", " powder_pattern.masks['zero_vanadium'],\n", diff --git a/packages/essreduce/src/ess/reduce/masking.py b/packages/essreduce/src/ess/reduce/masking.py index 4e4186708..21305e0bf 100644 --- a/packages/essreduce/src/ess/reduce/masking.py +++ b/packages/essreduce/src/ess/reduce/masking.py @@ -18,11 +18,12 @@ PixelMaskFilenames = NewType('PixelMaskFilenames', Sequence[str | Path]) """Files with pixel masks to apply. Use an empty sequence to apply none.""" -LoadedPixelMasks = NewType('LoadedPixelMasks', dict[str, sc.Variable | sc.DataGroup]) -"""Content of each mask file, by mask name. +PixelMasks = NewType('PixelMasks', dict[str, sc.Variable | sc.DataGroup]) +"""Pixel masks of the whole instrument, by mask name. -Either a variable of masked detector IDs, or a data group of boolean masks by detector -bank. +Each value is either a variable of masked detector IDs, or a data group of boolean masks +by detector bank. :py:func:`read_pixel_masks` provides them from files, and they can +also be set directly. :py:func:`to_detector_masks` selects the masks for one detector. """ _HDF5_SUFFIXES = ('.h5', '.hdf', '.hdf5', '.nxs') @@ -37,7 +38,7 @@ """ -def read_pixel_masks(filenames: PixelMaskFilenames) -> LoadedPixelMasks: +def read_pixel_masks(filenames: PixelMaskFilenames) -> PixelMasks: """Read pixel masks from files. The following formats are supported, selected by file suffix and content: @@ -85,11 +86,11 @@ def read_pixel_masks(filenames: PixelMaskFilenames) -> LoadedPixelMasks: if name in masks: raise ValueError(f"Several mask files give the mask name '{name}'.") masks[name] = content - return LoadedPixelMasks(masks) + return PixelMasks(masks) -def to_pixel_masks( - loaded: LoadedPixelMasks, +def to_detector_masks( + pixel_masks: PixelMasks, ids: sc.Variable, bank: str | None = None, known_banks: Collection[str] | None = None, @@ -101,8 +102,8 @@ def to_pixel_masks( Parameters ---------- - loaded: - Masks read from files. + pixel_masks: + Pixel masks of the instrument. ids: Detector IDs of the pixels. bank: @@ -112,7 +113,7 @@ def to_pixel_masks( bank raises an error. A misspelled name would otherwise mask nothing. """ masks = {} - for name, content in loaded.items(): + for name, content in pixel_masks.items(): if isinstance(content, sc.Variable): masks[name] = sc.array( dims=ids.dims, values=np.isin(ids.values, content.values) diff --git a/packages/essreduce/tests/masking_test.py b/packages/essreduce/tests/masking_test.py index c76a4aa8a..ad62f1de3 100644 --- a/packages/essreduce/tests/masking_test.py +++ b/packages/essreduce/tests/masking_test.py @@ -8,10 +8,10 @@ import scippnexus as snx from ess.reduce.masking import ( - LoadedPixelMasks, PixelMaskFilenames, + PixelMasks, read_pixel_masks, - to_pixel_masks, + to_detector_masks, write_pixel_masks, ) @@ -31,7 +31,7 @@ def _write_ids(path: Path, values: list[int]) -> Path: return path -def _read(*paths: Path) -> LoadedPixelMasks: +def _read(*paths: Path) -> PixelMasks: return read_pixel_masks(PixelMaskFilenames(paths)) @@ -174,7 +174,7 @@ def test_read_raises_if_file_has_no_pixel_mask(tmp_path: Path) -> None: def test_to_pixel_masks_keeps_shape_of_ids() -> None: ids = sc.array(dims=['tube', 'pixel'], values=[[1, 2], [3, 4]], unit=None) masked = sc.array(dims=['detector_id'], values=[2, 3, 99], unit=None) - masks = to_pixel_masks(LoadedPixelMasks({'m': masked}), ids) + masks = to_detector_masks(PixelMasks({'m': masked}), ids) expected = sc.array(dims=['tube', 'pixel'], values=[[False, True], [True, False]]) assert sc.identical(masks['m'], expected) @@ -184,8 +184,8 @@ def bank_ids() -> sc.Variable: return sc.array(dims=['tube', 'pixel'], values=[[1, 2], [3, 4], [5, 6]], unit=None) -def _per_bank(**banks: sc.Variable) -> LoadedPixelMasks: - return LoadedPixelMasks({'m': sc.DataGroup(banks)}) +def _per_bank(**banks: sc.Variable) -> PixelMasks: + return PixelMasks({'m': sc.DataGroup(banks)}) def test_to_pixel_masks_selects_bank_and_allows_fewer_dims( @@ -193,18 +193,18 @@ def test_to_pixel_masks_selects_bank_and_allows_fewer_dims( ) -> None: mask = sc.array(dims=['tube'], values=[True, False, False]) other = sc.array(dims=['tube'], values=[False, True, False]) - masks = to_pixel_masks(_per_bank(a=mask, b=other), bank_ids, bank='a') + masks = to_detector_masks(_per_bank(a=mask, b=other), bank_ids, bank='a') assert sc.identical(masks['m'], mask) def test_to_pixel_masks_skips_file_without_this_bank(bank_ids: sc.Variable) -> None: mask = sc.array(dims=['tube'], values=[True, False, False]) - assert to_pixel_masks(_per_bank(b=mask), bank_ids, bank='a') == {} + assert to_detector_masks(_per_bank(b=mask), bank_ids, bank='a') == {} def test_to_pixel_masks_applies_ids_to_any_bank(bank_ids: sc.Variable) -> None: masked = sc.array(dims=['detector_id'], values=[1], unit=None) - masks = to_pixel_masks(LoadedPixelMasks({'m': masked}), bank_ids, bank='a') + masks = to_detector_masks(PixelMasks({'m': masked}), bank_ids, bank='a') assert masks['m'].values.sum() == 1 @@ -213,13 +213,13 @@ def test_to_pixel_masks_raises_without_bank_for_masks_per_bank( ) -> None: mask = sc.array(dims=['tube'], values=[True, False, False]) with pytest.raises(ValueError, match='no bank is known'): - to_pixel_masks(_per_bank(a=mask), bank_ids) + to_detector_masks(_per_bank(a=mask), bank_ids) def test_to_pixel_masks_raises_on_unknown_bank(bank_ids: sc.Variable) -> None: mask = sc.array(dims=['tube'], values=[True, False, False]) with pytest.raises(ValueError, match='typo'): - to_pixel_masks( + to_detector_masks( _per_bank(a=mask, typo=mask), bank_ids, bank='a', known_banks={'a', 'b'} ) @@ -236,4 +236,4 @@ def test_to_pixel_masks_raises_if_mask_does_not_fit_detector( bank_ids: sc.Variable, mask: sc.Variable ) -> None: with pytest.raises(sc.DimensionError): - to_pixel_masks(_per_bank(a=mask), bank_ids, bank='a') + to_detector_masks(_per_bank(a=mask), bank_ids, bank='a') diff --git a/packages/esssans/src/ess/isissans/general.py b/packages/esssans/src/ess/isissans/general.py index 418e3b2b3..fd4f95dfc 100644 --- a/packages/esssans/src/ess/isissans/general.py +++ b/packages/esssans/src/ess/isissans/general.py @@ -19,13 +19,13 @@ EmptyDetector, EmptyMonitor, Incident, - LoadedPixelMasks, Measurement, MonitorPositionOffset, MonitorType, NeXusComponent, NeXusMonitorName, NonBackgroundWavelengthRange, + PixelMasks, RawDetector, RawMonitor, RunType, @@ -35,7 +35,7 @@ WavelengthMonitor, ) -from ess.reduce.masking import to_pixel_masks +from ess.reduce.masking import to_detector_masks from ess.reduce.nexus.types import NeXusTransformation, Position from .io import LoadedFileContents @@ -304,15 +304,13 @@ def get_detector_ids_from_sample_run(data: RawDetector[SampleRun]) -> DetectorID ) -def to_detector_mask_without_bank( - ids: DetectorIDs, masks: LoadedPixelMasks -) -> DetectorMasks: - """Create detector masks from masks read from files. +def to_detector_mask_without_bank(ids: DetectorIDs, masks: PixelMasks) -> DetectorMasks: + """Create detector masks from the pixel masks of the instrument. This overrides the function in the masking module, since files loaded using Mantid have no NeXus detector name. So mask files that give masks per bank are not usable. """ - return DetectorMasks(to_pixel_masks(masks, ids)) + return DetectorMasks(to_detector_masks(masks, ids)) providers = ( diff --git a/packages/esssans/src/ess/sans/masking.py b/packages/esssans/src/ess/sans/masking.py index ecce79c00..fa8f3bbf9 100644 --- a/packages/esssans/src/ess/sans/masking.py +++ b/packages/esssans/src/ess/sans/masking.py @@ -4,7 +4,7 @@ Masking functions for the loki workflow. """ -from ess.reduce.masking import read_pixel_masks, to_pixel_masks +from ess.reduce.masking import read_pixel_masks, to_detector_masks from .types import ( CorrectedDetector, @@ -12,9 +12,9 @@ DetectorIDs, DetectorMasks, EmptyDetector, - LoadedPixelMasks, NeXusDetectorName, Numerator, + PixelMasks, RunType, SampleRun, WavelengthDetector, @@ -32,24 +32,26 @@ def get_detector_ids_from_detector(data: EmptyDetector[SampleRun]) -> DetectorID def to_detector_mask( ids: DetectorIDs, - masks: LoadedPixelMasks, + masks: PixelMasks, bank: NeXusDetectorName, bank_sizes: DetectorBankSizes, ) -> DetectorMasks: - """Create detector masks for a bank from the masks read from files. + """Create detector masks for a bank from the pixel masks of the instrument. Parameters ---------- ids: Array of detector IDs masks: - Masks read from files. + Pixel masks of the instrument. bank: Name of the detector bank. bank_sizes: Sizes of the banks of the instrument, which define the valid bank names. """ - return DetectorMasks(to_pixel_masks(masks, ids, bank=bank, known_banks=bank_sizes)) + return DetectorMasks( + to_detector_masks(masks, ids, bank=bank, known_banks=bank_sizes) + ) def apply_pixel_masks( diff --git a/packages/esssans/src/ess/sans/types.py b/packages/esssans/src/ess/sans/types.py index 063fe9e3a..9c0efbe8c 100644 --- a/packages/esssans/src/ess/sans/types.py +++ b/packages/esssans/src/ess/sans/types.py @@ -182,7 +182,7 @@ class TransmissionRun(Generic[ScatteringRunType]): PixelMaskFilenames = masking_t.PixelMaskFilenames -LoadedPixelMasks = masking_t.LoadedPixelMasks +PixelMasks = masking_t.PixelMasks DetectorIDs = NewType('DetectorIDs', sc.Variable) """1-D variable listing all detector IDs.""" From 4e22fcf7c2cadc4fc27de94c94d2d80d63a7aa10 Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Mon, 9 Nov 2026 12:50:23 +0000 Subject: [PATCH 04/16] Check the types of pixel masks when converting them PixelMasks can be set directly, so its values are not limited to what the file readers produce. A boolean variable was treated as a list of detector IDs, so that [True] masked the pixel with ID 1, and an integer mask for a bank was passed on as a mask. Co-Authored-By: Claude Opus 5.5 --- packages/essreduce/src/ess/reduce/masking.py | 13 ++++++++++++- packages/essreduce/tests/masking_test.py | 16 ++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/packages/essreduce/src/ess/reduce/masking.py b/packages/essreduce/src/ess/reduce/masking.py index 21305e0bf..f154ae9ca 100644 --- a/packages/essreduce/src/ess/reduce/masking.py +++ b/packages/essreduce/src/ess/reduce/masking.py @@ -115,8 +115,9 @@ def to_detector_masks( masks = {} for name, content in pixel_masks.items(): if isinstance(content, sc.Variable): + masked_ids = _validate_masked_ids(content, name) masks[name] = sc.array( - dims=ids.dims, values=np.isin(ids.values, content.values) + dims=ids.dims, values=np.isin(ids.values, masked_ids.values) ) continue if bank is None: @@ -252,7 +253,17 @@ def _read_bank_mask(detector: snx.Group, filename: Path) -> sc.Variable: return sc.array(dims=field.dims, values=masked) +def _validate_masked_ids(masked_ids: sc.Variable, name: str) -> sc.Variable: + if masked_ids.dtype not in ('int32', 'int64'): + raise sc.DTypeError( + f"Mask '{name}' must hold integer detector IDs, not {masked_ids.dtype}." + ) + return masked_ids + + def _validate_bank_mask(mask: sc.Variable, ids: sc.Variable, name: str) -> sc.Variable: + if mask.dtype != 'bool': + raise sc.DTypeError(f"Mask '{name}' must be boolean, not {mask.dtype}.") # Masks with a dimension that the data lacks would be ignored without an error. if not mask.sizes.items() <= ids.sizes.items(): raise sc.DimensionError( diff --git a/packages/essreduce/tests/masking_test.py b/packages/essreduce/tests/masking_test.py index ad62f1de3..263a70fa6 100644 --- a/packages/essreduce/tests/masking_test.py +++ b/packages/essreduce/tests/masking_test.py @@ -224,6 +224,22 @@ def test_to_pixel_masks_raises_on_unknown_bank(bank_ids: sc.Variable) -> None: ) +def test_to_detector_masks_raises_if_masked_ids_are_not_integer( + bank_ids: sc.Variable, +) -> None: + masked = sc.array(dims=['x'], values=[True]) + with pytest.raises(sc.DTypeError, match='integer detector IDs'): + to_detector_masks(PixelMasks({'m': masked}), bank_ids) + + +def test_to_detector_masks_raises_if_bank_mask_is_not_boolean( + bank_ids: sc.Variable, +) -> None: + mask = sc.array(dims=['tube'], values=[0, 1, 2], unit=None) + with pytest.raises(sc.DTypeError, match='boolean'): + to_detector_masks(_per_bank(a=mask), bank_ids, bank='a') + + @pytest.mark.parametrize( 'mask', [ From dce147f8163a759224f2731099620fc4e7baff0a Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Mon, 9 Nov 2026 12:50:28 +0000 Subject: [PATCH 05/16] Apply pixel masks from files in the Sans2d workflow Sans2d builds DetectorMasks from its edge and sample holder masks only, so setting PixelMaskFilenames had no effect and gave no error. It now combines both. The list of files defaults to empty, so that the workflow works without mask files. Co-Authored-By: Claude Opus 5.5 --- packages/esssans/src/ess/isissans/sans2d.py | 43 +++++++++++++------ .../tests/isissans/sans2d_reduction_test.py | 34 +++++++++++++++ 2 files changed, 64 insertions(+), 13 deletions(-) diff --git a/packages/esssans/src/ess/isissans/sans2d.py b/packages/esssans/src/ess/isissans/sans2d.py index 253ff1240..310aa0c8a 100644 --- a/packages/esssans/src/ess/isissans/sans2d.py +++ b/packages/esssans/src/ess/isissans/sans2d.py @@ -6,11 +6,18 @@ import scipp as sc from ess.sans import SansWorkflow from ess.sans.parameters import typical_outputs -from ess.sans.types import DetectorMasks, EmptyDetector, SampleRun +from ess.sans.types import ( + DetectorIDs, + DetectorMasks, + EmptyDetector, + PixelMaskFilenames, + PixelMasks, + SampleRun, +) from ess.reduce.workflow import register_workflow -from .general import default_parameters +from .general import default_parameters, to_detector_mask_without_bank from .io import load_tutorial_direct_beam, load_tutorial_run from .mantidio import providers as mantid_providers @@ -50,26 +57,35 @@ def sample_holder_mask( def to_detector_masks( - edge_mask: DetectorEdgeMask, holder_mask: SampleHolderMask + ids: DetectorIDs, + pixel_masks: PixelMasks, + edge_mask: DetectorEdgeMask, + holder_mask: SampleHolderMask, ) -> DetectorMasks: - """Gather detector masks. - - Unlike :py:func:`ess.sans.masking.to_detector_mask`, this function does not use - mask files. Instead it directly returns a dictionary of multiple masks. + """Combine the pixel masks of the instrument with masks computed from the detector. Parameters ---------- + ids: + Array of detector IDs. + pixel_masks: + Pixel masks of the instrument. edge_mask: Mask for detector edges. holder_mask: Mask for sample holder. + + Raises + ------ + ValueError + If a pixel mask has the name of a computed mask, which would replace it. """ - masks = {} - if edge_mask is not None: - masks['edges'] = edge_mask - if holder_mask is not None: - masks['holder_mask'] = holder_mask - return DetectorMasks(masks) + masks = to_detector_mask_without_bank(ids, pixel_masks) + computed = {'edges': edge_mask, 'holder_mask': holder_mask} + computed = {name: mask for name, mask in computed.items() if mask is not None} + if clashes := masks.keys() & computed.keys(): + raise ValueError(f"Pixel masks use names of computed masks: {sorted(clashes)}.") + return DetectorMasks({**masks, **computed}) providers = (detector_edge_mask, sample_holder_mask, to_detector_masks) @@ -87,6 +103,7 @@ def Sans2dWorkflow() -> sciline.Pipeline: workflow.insert(provider) for key, param in default_parameters().items(): workflow[key] = param + workflow[PixelMaskFilenames] = PixelMaskFilenames([]) workflow.typical_outputs = typical_outputs return workflow diff --git a/packages/esssans/tests/isissans/sans2d_reduction_test.py b/packages/esssans/tests/isissans/sans2d_reduction_test.py index 8229bf1f6..25cf63e99 100644 --- a/packages/esssans/tests/isissans/sans2d_reduction_test.py +++ b/packages/esssans/tests/isissans/sans2d_reduction_test.py @@ -15,6 +15,8 @@ BeamCenter, CorrectedDetector, CorrectForGravity, + DetectorIDs, + DetectorMasks, DimsToKeep, DirectBeam, DirectBeamFilename, @@ -26,6 +28,8 @@ NeXusMonitorName, NonBackgroundWavelengthRange, Numerator, + PixelMaskFilenames, + PixelMasks, QBins, RawDetector, ReturnEvents, @@ -172,6 +176,36 @@ def test_pipeline_can_visualize_background_subtracted_IofQ(pipeline): pipeline.visualize(BackgroundSubtractedIofQ) +def test_detector_masks_default_to_computed_masks_only(pipeline): + assert set(pipeline.compute(DetectorMasks)) == {'edges', 'holder_mask'} + + +def test_mask_files_are_combined_with_computed_masks(pipeline, tmp_path): + detector_id = pipeline.compute(DetectorIDs).values[0] + path = tmp_path / 'broken.xml' + path.write_text( + '\n' + f'{detector_id}' + ) + pipeline[PixelMaskFilenames] = [path] + masks = pipeline.compute(DetectorMasks) + assert set(masks) == {'broken.xml', 'edges', 'holder_mask'} + assert masks['broken.xml'].values.sum() == 1 + + +def test_pixel_mask_with_name_of_computed_mask_raises(): + ids = sc.array(dims=['spectrum'], values=[1, 2], unit=None) + edge_mask = sc.array(dims=['spectrum'], values=[True, False]) + masked = sc.array(dims=['detector_id'], values=[1], unit=None) + with pytest.raises(ValueError, match='edges'): + sans2d.to_detector_masks( + DetectorIDs(ids), + PixelMasks({'edges': masked}), + sans2d.DetectorEdgeMask(edge_mask), + sans2d.SampleHolderMask(None), + ) + + def test_pipeline_can_compute_intermediate_results(pipeline): result = pipeline.compute(SolidAngle[SampleRun]) assert result.dims == ('spectrum',) From 560707550061d6b5d6bc42cb66373c8802375fa4 Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Mon, 9 Nov 2026 12:51:09 +0000 Subject: [PATCH 06/16] Use the name to_detector_masks in test names Co-Authored-By: Claude Opus 5.5 --- packages/essreduce/tests/masking_test.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/essreduce/tests/masking_test.py b/packages/essreduce/tests/masking_test.py index 263a70fa6..363e3071a 100644 --- a/packages/essreduce/tests/masking_test.py +++ b/packages/essreduce/tests/masking_test.py @@ -171,7 +171,7 @@ def test_read_raises_if_file_has_no_pixel_mask(tmp_path: Path) -> None: _read(path) -def test_to_pixel_masks_keeps_shape_of_ids() -> None: +def test_to_detector_masks_keeps_shape_of_ids() -> None: ids = sc.array(dims=['tube', 'pixel'], values=[[1, 2], [3, 4]], unit=None) masked = sc.array(dims=['detector_id'], values=[2, 3, 99], unit=None) masks = to_detector_masks(PixelMasks({'m': masked}), ids) @@ -188,7 +188,7 @@ def _per_bank(**banks: sc.Variable) -> PixelMasks: return PixelMasks({'m': sc.DataGroup(banks)}) -def test_to_pixel_masks_selects_bank_and_allows_fewer_dims( +def test_to_detector_masks_selects_bank_and_allows_fewer_dims( bank_ids: sc.Variable, ) -> None: mask = sc.array(dims=['tube'], values=[True, False, False]) @@ -197,18 +197,18 @@ def test_to_pixel_masks_selects_bank_and_allows_fewer_dims( assert sc.identical(masks['m'], mask) -def test_to_pixel_masks_skips_file_without_this_bank(bank_ids: sc.Variable) -> None: +def test_to_detector_masks_skips_file_without_this_bank(bank_ids: sc.Variable) -> None: mask = sc.array(dims=['tube'], values=[True, False, False]) assert to_detector_masks(_per_bank(b=mask), bank_ids, bank='a') == {} -def test_to_pixel_masks_applies_ids_to_any_bank(bank_ids: sc.Variable) -> None: +def test_to_detector_masks_applies_ids_to_any_bank(bank_ids: sc.Variable) -> None: masked = sc.array(dims=['detector_id'], values=[1], unit=None) masks = to_detector_masks(PixelMasks({'m': masked}), bank_ids, bank='a') assert masks['m'].values.sum() == 1 -def test_to_pixel_masks_raises_without_bank_for_masks_per_bank( +def test_to_detector_masks_raises_without_bank_for_masks_per_bank( bank_ids: sc.Variable, ) -> None: mask = sc.array(dims=['tube'], values=[True, False, False]) @@ -216,7 +216,7 @@ def test_to_pixel_masks_raises_without_bank_for_masks_per_bank( to_detector_masks(_per_bank(a=mask), bank_ids) -def test_to_pixel_masks_raises_on_unknown_bank(bank_ids: sc.Variable) -> None: +def test_to_detector_masks_raises_on_unknown_bank(bank_ids: sc.Variable) -> None: mask = sc.array(dims=['tube'], values=[True, False, False]) with pytest.raises(ValueError, match='typo'): to_detector_masks( @@ -248,7 +248,7 @@ def test_to_detector_masks_raises_if_bank_mask_is_not_boolean( ], ids=['unknown-dim', 'wrong-size'], ) -def test_to_pixel_masks_raises_if_mask_does_not_fit_detector( +def test_to_detector_masks_raises_if_mask_does_not_fit_detector( bank_ids: sc.Variable, mask: sc.Variable ) -> None: with pytest.raises(sc.DimensionError): From 2a4b6c77e14444218745cb861850db845736c690 Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Mon, 9 Nov 2026 13:38:03 +0000 Subject: [PATCH 07/16] Record why mask files do not use NeXus start_time and end_time Co-Authored-By: Claude Opus 5.5 --- .../docs/developer/adr/0001-pixel-mask-file-format.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md b/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md index 8c15c16da..6a1cdc68e 100644 --- a/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md +++ b/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md @@ -74,9 +74,18 @@ New files should not use scipp HDF5. It cannot express a mask of a tube or a straw except by listing every pixel. - **Workflows check the validity date.** This would require each workflow to know the time of a run and how to handle expired masks, while a list of filenames is enough for it to do its job. +- **The NeXus fields `start_time` and `end_time` for the validity period.** + NeXus defines them as the start and end of a measurement, which can differ from the validity of a mask. + For example, a mask derived from a run in November can be valid from the day in October when a tube broke. + A reader of an archived file would also take them for measurement times. + `end_time` is usually unknown when a file is archived, since a static mask stops being valid when the hardware changes, and the file cannot be edited then. ## Consequences +- `valid_from` is provisional. + Nothing reads it yet. + The software that selects mask files for a run may need a different field, or may keep the date in catalog metadata. + - The format goes beyond the NeXus definition in three places. NeXus describes `pixel_mask` with rank 2, while masks here have the rank of the logical dimensions they use. Dimension labels are an HDF5 feature, not defined by NeXus. From f37f2df3d5033831d97f32a145ac351c5dd7efd2 Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Mon, 9 Nov 2026 13:40:55 +0000 Subject: [PATCH 08/16] Reject pixel mask files with several NXentry groups The reader looks only at /entry. Any other entry would be ignored without an error, which would leave pixels unmasked. This also applies if files with several entries are supported later, and an older reader gets one of them. The ADR describes that possible extension. Co-Authored-By: Claude Opus 5.5 --- .../adr/0001-pixel-mask-file-format.md | 24 +++++++++++++++++++ packages/essreduce/src/ess/reduce/masking.py | 3 +++ packages/essreduce/tests/masking_test.py | 9 +++++++ 3 files changed, 36 insertions(+) diff --git a/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md b/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md index 6a1cdc68e..f13d89a53 100644 --- a/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md +++ b/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md @@ -51,6 +51,7 @@ with h5py.File(path) as f: - Each file gives one mask per bank. Tube and straw masks combine into one `(layer, tube, straw)` array. The `pixel_mask_N` fields of NeXus are an error, so that they are not ignored. + A file with several `NXentry` groups is an error for the same reason. - `title` becomes the name of the mask in the reduced data. Without it, the name of the file is used. Two masks with the same name are an error. @@ -95,3 +96,26 @@ New files should not use scipp HDF5. Mask files must be recreated when the layout of a bank changes. - Mask files have no format version. A later incompatible change needs a different location than `/entry/instrument//pixel_mask`. + +## Possible extension: several masks in one file + +A file could hold several masks as several `NXentry` groups, which NeXus allows: + +```text +/broken_tubes NXentry + title "broken-tubes" + instrument/loki_detector_0/pixel_mask dims ("tube") +/beamstop NXentry + title "beamstop" + instrument/loki_detector_0/pixel_mask dims ("layer", "tube", "straw") +``` + +Each entry would be one named mask, with its own `title` and `valid_from`, and with its own dimensions. +Tube and straw masks would then not have to be combined into one array. +Only the reader would change, and files with one entry would stay valid. + +This is not implemented, because a list of several files already gives several named masks. +Files with several entries are rejected today, so that a version of the reader without this extension raises an error instead of leaving pixels unmasked. + +The `pixel_mask_N` fields are not suitable for this extension. +NeXus defines them as parts of one mask, combined with a bitwise OR, and they have no names. diff --git a/packages/essreduce/src/ess/reduce/masking.py b/packages/essreduce/src/ess/reduce/masking.py index f154ae9ca..502b34cc4 100644 --- a/packages/essreduce/src/ess/reduce/masking.py +++ b/packages/essreduce/src/ess/reduce/masking.py @@ -214,6 +214,9 @@ def _read_mask_file(filename: Path) -> tuple[str, sc.Variable | sc.DataGroup]: def _read_hdf5(filename: Path) -> tuple[str, sc.Variable | sc.DataGroup]: with snx.File(filename) as file: + # Reading only one of several entries would leave pixels unmasked. + if len(file[snx.NXentry]) > 1: + raise ValueError(f"Mask file '{filename}' has several NXentry groups.") if 'entry' in file: return _read_nexus_masks(file['entry'], filename) ids = sc.io.load_hdf5(filename) diff --git a/packages/essreduce/tests/masking_test.py b/packages/essreduce/tests/masking_test.py index 363e3071a..246c921dd 100644 --- a/packages/essreduce/tests/masking_test.py +++ b/packages/essreduce/tests/masking_test.py @@ -165,6 +165,15 @@ def test_read_raises_on_numbered_pixel_masks(tmp_path: Path) -> None: _read(path) +def test_read_raises_on_several_entries(tmp_path: Path) -> None: + path = tmp_path / 'a.nxs' + write_pixel_masks(path, {'bank_0': sc.array(dims=['tube'], values=[True])}) + with snx.File(path, 'r+') as file: + file.create_class('other', snx.NXentry) + with pytest.raises(ValueError, match='several NXentry'): + _read(path) + + def test_read_raises_if_file_has_no_pixel_mask(tmp_path: Path) -> None: path = _write_raw_pixel_mask(tmp_path / 'a.nxs', 'detector_number', np.array([1])) with pytest.raises(ValueError, match='no detector with a pixel_mask'): From f557f63dd7f4861f9b03ced25f25e38de474f2ac Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Mon, 9 Nov 2026 13:44:32 +0000 Subject: [PATCH 09/16] Record why mask files do not use the NeXus axes attribute Co-Authored-By: Claude Opus 5.5 --- .../docs/developer/adr/0001-pixel-mask-file-format.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md b/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md index f13d89a53..57d0f3fb5 100644 --- a/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md +++ b/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md @@ -70,6 +70,10 @@ New files should not use scipp HDF5. - **HDF5 with our own group names and a boolean dataset.** It would need its own specification of what NeXus already defines. HDF5 also has no boolean type: h5py stores booleans as an enumeration, which is a convention of h5py. +- **The NeXus attribute `axes` for the names of dimensions, instead of or in addition to HDF5 dimension labels.** + NeXus defines `axes` on `NXdata`, as the names of the coordinate fields along each dimension of the signal. + `NXdetector` does not define it, and a mask has no coordinates, so a correct value would be `"."` for each dimension, which holds no names. + The names decide how a mask of lower rank applies to the pixels, so they should be stored in one place only. - **Lists of detector IDs, for new files too.** `detector_number` is unique across the banks of LoKI, so such a file needs no bank level, and it stays correct if the layout of a bank changes. It cannot express a mask of a tube or a straw except by listing every pixel. From f2f69286ae84decb566693f6f943498cef7931bc Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Mon, 9 Nov 2026 13:53:00 +0000 Subject: [PATCH 10/16] Give detector masks the dimension order of the detector A mask file may use any order of dimensions, so DetectorMasks could have a different layout depending on the file. It now always follows the detector. Add tests for masks with transposed dimensions or a subset of them, and for files whose sizes or labels do not fit the detector, including a comparison of I(Q) with an equivalent list of detector IDs. Co-Authored-By: Claude Opus 5.5 --- .../adr/0001-pixel-mask-file-format.md | 2 +- packages/essreduce/src/ess/reduce/masking.py | 16 ++--- packages/essreduce/tests/masking_test.py | 60 ++++++++++++++++++- packages/esssans/tests/loki/workflow_test.py | 43 +++++++++++++ 4 files changed, 110 insertions(+), 11 deletions(-) diff --git a/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md b/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md index 57d0f3fb5..2c90085bd 100644 --- a/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md +++ b/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md @@ -46,7 +46,7 @@ with h5py.File(path) as f: The other bits are tags, which do not mask a pixel. `ess.reduce.masking.write_pixel_masks` sets bit 8, "user defined mask". - Dimensions of `pixel_mask` are named with HDF5 dimension labels. - They are a subset of the logical dimensions of the bank, with matching sizes. + They are a subset of the logical dimensions of the bank, in any order, with matching sizes. This is checked when masks are applied, because scipp ignores a mask with a dimension that the data lacks, without an error. - Each file gives one mask per bank. Tube and straw masks combine into one `(layer, tube, straw)` array. diff --git a/packages/essreduce/src/ess/reduce/masking.py b/packages/essreduce/src/ess/reduce/masking.py index 502b34cc4..785e7c6e9 100644 --- a/packages/essreduce/src/ess/reduce/masking.py +++ b/packages/essreduce/src/ess/reduce/masking.py @@ -61,8 +61,8 @@ def read_pixel_masks(filenames: PixelMaskFilenames) -> PixelMasks: ``pixel_mask`` has no mask from this file. Following NeXus, a pixel is masked if any bit in ``pixel_mask & 0xFFFF`` is set. The dimensions of ``pixel_mask`` are given by HDF5 dimension labels. They are named after the logical dimensions - of the bank, and may be a subset of them. Other entries of the file, such as a - validity date, are ignored. + of the bank, and may be a subset of them, in any order. Other entries of the + file, such as a validity date, are ignored. - A file written by :py:func:`scipp.io.save_hdf5` with a variable of masked detector IDs. @@ -98,7 +98,8 @@ def to_detector_masks( """Create boolean masks for the pixels of a detector bank. Masks given as detector IDs apply to any bank, since IDs identify a pixel. They - have the dimensions of ``ids``. Masks given per bank are selected by ``bank``. + have the dimensions of ``ids``. Masks given per bank are selected by ``bank``, + and follow the dimension order of ``ids``. Parameters ---------- @@ -125,7 +126,7 @@ def to_detector_masks( if known_banks is not None and (unknown := set(content) - set(known_banks)): raise ValueError(f"Mask '{name}' has unknown banks: {sorted(unknown)}.") if bank in content: - masks[name] = _validate_bank_mask(content[bank], ids, name) + masks[name] = _align_bank_mask(content[bank], ids, name) return masks @@ -144,7 +145,7 @@ def write_pixel_masks( Path of the file to write. masks: Boolean mask of each detector bank. Dimensions are named after the logical - dimensions of the bank, and may be a subset of them. + dimensions of the bank, and may be a subset of them, in any order. name: Name of the mask. Without it, the mask is named after the file when it is read. """ @@ -264,7 +265,7 @@ def _validate_masked_ids(masked_ids: sc.Variable, name: str) -> sc.Variable: return masked_ids -def _validate_bank_mask(mask: sc.Variable, ids: sc.Variable, name: str) -> sc.Variable: +def _align_bank_mask(mask: sc.Variable, ids: sc.Variable, name: str) -> sc.Variable: if mask.dtype != 'bool': raise sc.DTypeError(f"Mask '{name}' must be boolean, not {mask.dtype}.") # Masks with a dimension that the data lacks would be ignored without an error. @@ -273,4 +274,5 @@ def _validate_bank_mask(mask: sc.Variable, ids: sc.Variable, name: str) -> sc.Va f"Mask '{name}' has sizes {mask.sizes}, which are not part of the sizes " f"of the detector, {ids.sizes}." ) - return mask + # A mask file may use any order. DetectorMasks then has one layout for all of them. + return mask.transpose([dim for dim in ids.dims if dim in mask.dims]) diff --git a/packages/essreduce/tests/masking_test.py b/packages/essreduce/tests/masking_test.py index 246c921dd..c54e3f1b6 100644 --- a/packages/essreduce/tests/masking_test.py +++ b/packages/essreduce/tests/masking_test.py @@ -35,13 +35,15 @@ def _read(*paths: Path) -> PixelMasks: return read_pixel_masks(PixelMaskFilenames(paths)) -def _write_raw_pixel_mask(path: Path, field: str, values: np.ndarray) -> Path: +def _write_raw_pixel_mask( + path: Path, field: str, values: np.ndarray, label: str = 'tube' +) -> Path: with snx.File(path, 'w') as file: entry = file.create_class('entry', snx.NXentry) instrument = entry.create_class('instrument', snx.NXinstrument) detector = instrument.create_class('bank_0', snx.NXdetector) dataset = detector.create_field(field, values) - dataset.dims[0].label = 'tube' + dataset.dims[0].label = label return path @@ -254,11 +256,63 @@ def test_to_detector_masks_raises_if_bank_mask_is_not_boolean( [ sc.array(dims=['straw'], values=[True, False, False]), sc.array(dims=['tube'], values=[True, False]), + sc.array(dims=['pixel', 'tube'], values=[[True, False]] * 3), + sc.array(dims=['tube', 'straw'], values=[[True], [False], [False]]), ], - ids=['unknown-dim', 'wrong-size'], + ids=['unknown-dim', 'wrong-size', 'sizes-of-other-dim', 'extra-dim'], ) def test_to_detector_masks_raises_if_mask_does_not_fit_detector( bank_ids: sc.Variable, mask: sc.Variable ) -> None: with pytest.raises(sc.DimensionError): to_detector_masks(_per_bank(a=mask), bank_ids, bank='a') + + +def test_to_detector_masks_raises_if_file_has_no_dimension_labels( + bank_ids: sc.Variable, tmp_path: Path +) -> None: + values = np.array([0, 1, 0]) + path = _write_raw_pixel_mask(tmp_path / 'a.nxs', 'pixel_mask', values, label='') + with pytest.raises(sc.DimensionError): + to_detector_masks(_read(path), bank_ids, bank='bank_0') + + +@pytest.mark.parametrize( + ('mask_dims', 'expected_dims'), + [ + (('layer', 'tube', 'straw'), ('layer', 'tube', 'straw')), + (('straw', 'tube', 'layer'), ('layer', 'tube', 'straw')), + (('tube', 'straw', 'layer'), ('layer', 'tube', 'straw')), + (('tube',), ('tube',)), + (('layer', 'straw'), ('layer', 'straw')), + (('straw', 'layer'), ('layer', 'straw')), + (('straw', 'tube'), ('tube', 'straw')), + ], + ids=[ + 'full', + 'full-reversed', + 'full-reordered', + 'one-dim', + 'skips-dim', + 'skips-dim-transposed', + 'subset-transposed', + ], +) +def test_to_detector_masks_follows_dimension_order_of_detector( + mask_dims: tuple[str, ...], expected_dims: tuple[str, ...] +) -> None: + sizes = {'layer': 2, 'tube': 3, 'straw': 4} + ids = sc.array(dims=list(sizes), values=np.arange(24).reshape(2, 3, 4), unit=None) + shape = [sizes[dim] for dim in mask_dims] + values = np.arange(np.prod(shape)).reshape(shape) % 3 == 0 + mask = sc.array(dims=list(mask_dims), values=values) + result = to_detector_masks(_per_bank(a=mask), ids, bank='a')['m'] + assert result.dims == expected_dims + assert sc.identical(result, mask.transpose(list(expected_dims))) + + +def test_to_detector_masks_accepts_scalar_for_whole_bank( + bank_ids: sc.Variable, +) -> None: + result = to_detector_masks(_per_bank(a=sc.scalar(True)), bank_ids, bank='a')['m'] + assert sc.identical(result, sc.scalar(True)) diff --git a/packages/esssans/tests/loki/workflow_test.py b/packages/esssans/tests/loki/workflow_test.py index 4bc0b8e2e..6cdf6a348 100644 --- a/packages/esssans/tests/loki/workflow_test.py +++ b/packages/esssans/tests/loki/workflow_test.py @@ -184,6 +184,49 @@ def test_loki_workflow_raises_if_mask_file_has_unknown_bank(tmp_path): wf.compute(DetectorMasks) +@pytest.mark.parametrize( + 'dims', + [ + ('pixel', 'straw', 'tube', 'layer'), + ('layer', 'straw'), + ('straw', 'layer'), + ], + ids=['full-reversed', 'skips-dims', 'skips-dims-transposed'], +) +def test_bank_mask_gives_same_iofq_as_equivalent_id_file( + larmor_workflow, tmp_path, dims +): + wf = larmor_workflow(no_masks=False) + wf[UncertaintyBroadcastMode] = UncertaintyBroadcastMode.drop + # For simplicity, insert a fake beam center instead of computing it. + wf[BeamCenter] = sc.vector([0.0, 0.0, 0.0], unit='m') + ids = wf.compute(DetectorIDs) + + shape = [ids.sizes[dim] for dim in dims] + mask = sc.array( + dims=list(dims), values=np.arange(np.prod(shape)).reshape(shape) % 5 == 0 + ) + expanded = sc.broadcast(mask, sizes=ids.sizes).transpose(ids.dims) + masked_ids = sc.array( + dims=['detector_id'], + values=ids.values[expanded.values], + unit=None, + dtype='int32', + ) + assert 0 < masked_ids.size < ids.size + + bank_file = tmp_path / 'bank.nxs' + write_pixel_masks(bank_file, {'larmor_detector': mask}) + id_file = tmp_path / 'ids.h5' + sc.io.save_hdf5(masked_ids, id_file) + + wf[PixelMaskFilenames] = [bank_file] + from_bank_mask = wf.compute(BackgroundSubtractedIofQ) + wf[PixelMaskFilenames] = [id_file] + from_ids = wf.compute(BackgroundSubtractedIofQ) + assert sc.identical(from_bank_mask, from_ids) + + def test_bool_mask_file_gives_same_masks_as_id_file(larmor_workflow, tmp_path): wf = larmor_workflow(no_masks=False) wf[PixelMaskFilenames] = loki.data.loki_tutorial_mask_filenames() From f7b2eadb288d97bba5a4ed00a973f565ef4d098e Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Tue, 10 Nov 2026 09:56:35 +0000 Subject: [PATCH 11/16] Store named masks in an NXcollection per detector bank Mask files hold any number of named masks, as datasets in /entry/instrument//masks (NXcollection), instead of one NeXus pixel_mask bit field per bank. The dataset name is the mask name, a nonzero value masks a pixel, and each mask has its own dimensions. This allows one file for all masks of an instrument, and tube and straw masks no longer have to be combined into one array. Masks with different validity periods go into separate files; workflows use the masks of all files and raise on duplicate names. The writer stores uint8, since HDF5 has no boolean type. The reader accepts any integer type and the h5py boolean enumeration. Co-Authored-By: Claude Opus 5.5 --- .../tests/powder/masking_test.py | 2 +- .../adr/0001-pixel-mask-file-format.md | 106 ++++++------ packages/essreduce/src/ess/reduce/masking.py | 151 +++++++++--------- packages/essreduce/tests/masking_test.py | 128 ++++++++------- packages/esssans/tests/loki/workflow_test.py | 10 +- 5 files changed, 200 insertions(+), 197 deletions(-) diff --git a/packages/essdiffraction/tests/powder/masking_test.py b/packages/essdiffraction/tests/powder/masking_test.py index eb7f2cc25..2263a94b4 100644 --- a/packages/essdiffraction/tests/powder/masking_test.py +++ b/packages/essdiffraction/tests/powder/masking_test.py @@ -7,7 +7,7 @@ from ess.powder.types import PixelMaskFilenames -def test_apply_masks_masks_pixels_listed_in_file(tmp_path: Path) -> None: +def test_apply_masks_sets_mask_of_pixels_listed_in_file(tmp_path: Path) -> None: path = tmp_path / 'broken.h5' masked = sc.array(dims=['id'], values=[2], unit=None, dtype='int32') sc.io.save_hdf5(masked, path) diff --git a/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md b/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md index 2c90085bd..d0c206ed7 100644 --- a/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md +++ b/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md @@ -7,6 +7,7 @@ Instrument scientists will create pixel masks, which are stored as files and used by reduction workflows for years. Some are static, such as broken tubes, and are replaced when the hardware changes (see [#767](https://github.com/scipp/ess/issues/767)). +Others depend on the experiment, such as the shadow of a beamstop. A file that exists in a data catalog cannot be changed by a software release, so this format is harder to change than the code that reads it. The files must remain readable without ESS software or a particular version of it. @@ -16,60 +17,75 @@ The planned masking tool for LoKI works on tubes and straws of a bank, after the ## Decision -A mask file is a NeXus file, which is an HDF5 file with a defined structure: +A mask file is an HDF5 file with the group structure of NeXus: ```text /entry NXentry - title "broken-tubes", optional valid_from "2026-11-01T00:00:00Z", not read by workflows instrument NXinstrument loki_detector_0 NXdetector - pixel_mask int32, HDF5 dimension labels ("layer", "tube") + masks NXcollection + broken_tubes uint8, HDF5 dimension labels ("tube",) + beamstop uint8, HDF5 dimension labels ("layer", "tube", "straw") loki_detector_1 NXdetector - pixel_mask ... + masks NXcollection + broken_tubes ... ``` The content can be read with any HDF5 library: ```python with h5py.File(path) as f: - dataset = f['entry/instrument/loki_detector_0/pixel_mask'] - masked = (dataset[()] & 0xFFFF) != 0 + dataset = f['entry/instrument/loki_detector_0/masks/broken_tubes'] + masked = dataset[()] != 0 dims = [dim.label for dim in dataset.dims] ``` - Names of `NXdetector` groups are the names of the groups in the NeXus files of the runs. - A bank without `pixel_mask` has no mask from this file. A name that is not a bank of the instrument is an error, since a misspelled name would otherwise mask nothing. -- `pixel_mask` is the integer bit field defined by NeXus for [`NXdetector`](https://manual.nexusformat.org/classes/base_classes/NXdetector.html). - A pixel is masked if any bit in `pixel_mask & 0xFFFF` is set, as NeXus specifies. - The other bits are tags, which do not mask a pixel. - `ess.reduce.masking.write_pixel_masks` sets bit 8, "user defined mask". -- Dimensions of `pixel_mask` are named with HDF5 dimension labels. +- The name of a dataset in `masks` is the name of the mask in the reduced data. + Datasets with the same name in several banks are one mask. + A bank without a dataset of that name has no pixels masked by it. +- A pixel is masked if its value is not zero. + `ess.reduce.masking.write_pixel_masks` writes `uint8`. + Readers accept any integer type, and the boolean enumeration that h5py writes for numpy booleans. +- Dimensions of a mask are named with HDF5 dimension labels. They are a subset of the logical dimensions of the bank, in any order, with matching sizes. - This is checked when masks are applied, because scipp ignores a mask with a dimension that the data lacks, without an error. -- Each file gives one mask per bank. - Tube and straw masks combine into one `(layer, tube, straw)` array. - The `pixel_mask_N` fields of NeXus are an error, so that they are not ignored. - A file with several `NXentry` groups is an error for the same reason. -- `title` becomes the name of the mask in the reduced data. - Without it, the name of the file is used. - Two masks with the same name are an error. - -Workflows take a list of filenames and read all `pixel_mask` fields. -They do not check validity dates. + Each mask has its own dimensions, so a tube mask and a straw mask need not be combined into one array. + The sizes are checked when masks are applied, because scipp ignores a mask with a dimension that the data lacks, without an error. +- A file with several `NXentry` groups is an error, since the reader would use only one of them and leave pixels unmasked. + +Workflows take a list of filenames and use the masks of all files. +Two files with a mask of the same name are an error. +A file can hold all masks of an instrument, or masks can be split into files, for example by validity period: one file for broken tubes and one for the beamstop of an experiment. +Workflows do not check validity dates. Selecting the files that are valid for a run is left to the caller of the workflow, which sets the list of filenames. The readers for the existing formats stay, since they carry a list of detector IDs that works without a bank name. +Such a mask is named after its file. New files should not use scipp HDF5. ## Alternatives considered +- **The NeXus field `pixel_mask` of `NXdetector`.** + This is the field NeXus defines for masks: a bit field where a pixel is masked if any bit in `pixel_mask & 0xFFFF` is set. + It allows one mask per bank, with no name. + The additional fields `pixel_mask_N` have no names either, and NeXus defines them as parts of one mask, combined with a bitwise OR. + A file could therefore hold only one named mask, and a tube mask and a straw mask of the same bank would have to be combined into one array. + Naming masks by file or by `NXentry` title would work around this, at the cost of many files per instrument. + NeXus also describes `pixel_mask` with rank 2, while our masks have the rank of the logical dimensions they use, so we would deviate from the definition anyway. + No tool outside ESS is expected to read these files, so following the definition gains little. + The name `pixel_mask` is also confusing for LoKI, which has a dimension `pixel`. +- **Masks as datasets directly in `NXdetector`, without the `masks` group.** + NeXus defines many fields of `NXdetector`, such as `data`, `detector_number`, `depends_on` and `pixel_mask`. + Mask names would have to avoid all of them, and a reader could not tell a mask from another field. + NeXus describes `NXcollection` as the place for terms that it does not define, and the group keeps masks apart if a file ever holds other fields of a detector, such as geometry. - **A scipp HDF5 file holding a `DataGroup`.** This needs no writer, but the layout is defined by the code of scipp and not by a specification, so an archived file would depend on that code. -- **HDF5 with our own group names and a boolean dataset.** - It would need its own specification of what NeXus already defines. - HDF5 also has no boolean type: h5py stores booleans as an enumeration, which is a convention of h5py. +- **Boolean datasets.** + HDF5 has no boolean type. + h5py stores numpy booleans as an enumeration, which is a convention of h5py. + `uint8` is a standard HDF5 type that every HDF5 library reads, and it has the same size. - **The NeXus attribute `axes` for the names of dimensions, instead of or in addition to HDF5 dimension labels.** NeXus defines `axes` on `NXdata`, as the names of the coordinate fields along each dimension of the signal. `NXdetector` does not define it, and a mask has no coordinates, so a correct value would be `"."` for each dimension, which holds no names. @@ -87,39 +103,13 @@ New files should not use scipp HDF5. ## Consequences -- `valid_from` is provisional. - Nothing reads it yet. - The software that selects mask files for a run may need a different field, or may keep the date in catalog metadata. - -- The format goes beyond the NeXus definition in three places. - NeXus describes `pixel_mask` with rank 2, while masks here have the rank of the logical dimensions they use. - Dimension labels are an HDF5 feature, not defined by NeXus. - `valid_from` is not a field that NeXus defines for `NXentry`. +- The file uses the group classes of NeXus, but the masks are not fields that NeXus defines. + A generic NeXus tool shows them as datasets of an `NXcollection`, without knowing that they are masks. +- `valid_from` applies to all masks in a file. + Masks with different validity periods go into different files. + `valid_from` is provisional: nothing reads it yet, and the software that selects mask files for a run may need a different field, or may keep the date in catalog metadata. - Boolean masks are positional. A change of a bank's layout that keeps its shape would apply old masks to the wrong pixels without an error. Mask files must be recreated when the layout of a bank changes. - Mask files have no format version. - A later incompatible change needs a different location than `/entry/instrument//pixel_mask`. - -## Possible extension: several masks in one file - -A file could hold several masks as several `NXentry` groups, which NeXus allows: - -```text -/broken_tubes NXentry - title "broken-tubes" - instrument/loki_detector_0/pixel_mask dims ("tube") -/beamstop NXentry - title "beamstop" - instrument/loki_detector_0/pixel_mask dims ("layer", "tube", "straw") -``` - -Each entry would be one named mask, with its own `title` and `valid_from`, and with its own dimensions. -Tube and straw masks would then not have to be combined into one array. -Only the reader would change, and files with one entry would stay valid. - -This is not implemented, because a list of several files already gives several named masks. -Files with several entries are rejected today, so that a version of the reader without this extension raises an error instead of leaving pixels unmasked. - -The `pixel_mask_N` fields are not suitable for this extension. -NeXus defines them as parts of one mask, combined with a bitwise OR, and they have no names. + A later incompatible change needs a different location than `/entry/instrument//masks`. diff --git a/packages/essreduce/src/ess/reduce/masking.py b/packages/essreduce/src/ess/reduce/masking.py index 785e7c6e9..f7b3939e6 100644 --- a/packages/essreduce/src/ess/reduce/masking.py +++ b/packages/essreduce/src/ess/reduce/masking.py @@ -6,7 +6,6 @@ once. Converting the file content to masks for a detector is a separate step. """ -import re from collections.abc import Collection, Mapping, Sequence from pathlib import Path from typing import NewType @@ -28,15 +27,6 @@ _HDF5_SUFFIXES = ('.h5', '.hdf', '.hdf5', '.nxs') -_USER_DEFINED_MASK = 1 << 8 -"""Bit of the NeXus ``pixel_mask`` field for a mask defined by the user.""" - -_REJECTING_BITS = 0xFFFF -"""Bits of the NeXus ``pixel_mask`` field that exclude a pixel from analysis. - -Other bits are tags, which do not exclude a pixel on their own. -""" - def read_pixel_masks(filenames: PixelMaskFilenames) -> PixelMasks: """Read pixel masks from files. @@ -47,27 +37,28 @@ def read_pixel_masks(filenames: PixelMaskFilenames) -> PixelMasks: see :py:func:`read_xml_masked_detector_ids`. - ``.h5``, ``.hdf``, ``.hdf5``, ``.nxs``: Either of - - A NeXus file with masks by detector bank, see :py:func:`write_pixel_masks`: + - A NeXus file with named masks by detector bank, + see :py:func:`write_pixel_masks`: .. code-block:: text /entry (NXentry) - title name of the mask, optional instrument (NXinstrument) loki_detector_0 (NXdetector) - pixel_mask integer array - - Bank names are the names of the NeXus detector groups. A bank without - ``pixel_mask`` has no mask from this file. Following NeXus, a pixel is masked - if any bit in ``pixel_mask & 0xFFFF`` is set. The dimensions of ``pixel_mask`` - are given by HDF5 dimension labels. They are named after the logical dimensions - of the bank, and may be a subset of them, in any order. Other entries of the - file, such as a validity date, are ignored. + masks (NXcollection) + broken_tubes integer array + beamstop integer array + + Bank names are the names of the NeXus detector groups, and mask names are the + names of the datasets. A mask with the same name in several banks is one mask. + A bank without a dataset of that name has no pixels masked by it. A pixel is + masked if its value is not zero. The dimensions of a dataset are given by HDF5 + dimension labels. They are named after the logical dimensions of the bank, and + may be a subset of them, in any order. Other entries of the file, such as a + validity date, are ignored. - A file written by :py:func:`scipp.io.save_hdf5` with a variable of masked - detector IDs. - - A mask has the name stored in its file. Otherwise it is named after its file, - without the directory, since directories differ between hosts. + detector IDs. The mask is named after the file, without the directory, since + directories differ between hosts. The same applies to ``.xml`` files. Parameters ---------- @@ -82,10 +73,10 @@ def read_pixel_masks(filenames: PixelMaskFilenames) -> PixelMasks: """ masks = {} for filename in map(Path, filenames): - name, content = _read_mask_file(filename) - if name in masks: - raise ValueError(f"Several mask files give the mask name '{name}'.") - masks[name] = content + for name, content in _read_mask_file(filename).items(): + if name in masks: + raise ValueError(f"Several mask files give the mask name '{name}'.") + masks[name] = content return PixelMasks(masks) @@ -97,9 +88,14 @@ def to_detector_masks( ) -> dict[str, sc.Variable]: """Create boolean masks for the pixels of a detector bank. - Masks given as detector IDs apply to any bank, since IDs identify a pixel. They - have the dimensions of ``ids``. Masks given per bank are selected by ``bank``, - and follow the dimension order of ``ids``. + The masks use the logical dimensions of the bank, which are the dimensions of + ``ids``: + + - Masks given per bank are selected by ``bank``. They keep their dimensions, which + may be a subset of those of ``ids``, and are transposed to the order of ``ids``. + - Masks given as a list of detector IDs, such as ISIS XML files, apply to any bank, + since IDs identify a pixel. They are converted to a mask with the dimensions of + ``ids`` by looking up each pixel's ID in the list. Parameters ---------- @@ -131,38 +127,35 @@ def to_detector_masks( def write_pixel_masks( - filename: str | Path, - masks: Mapping[str, sc.Variable], - name: str | None = None, + filename: str | Path, masks: Mapping[str, Mapping[str, sc.Variable]] ) -> None: - """Write boolean masks by detector bank to a NeXus file. + """Write named boolean masks by detector bank to a NeXus file. - See :py:func:`read_pixel_masks` for the layout of the file. + See :py:func:`read_pixel_masks` for the layout of the file. Masks are stored as + unsigned 8-bit integers, since HDF5 has no boolean type. Parameters ---------- filename: Path of the file to write. masks: - Boolean mask of each detector bank. Dimensions are named after the logical - dimensions of the bank, and may be a subset of them, in any order. - name: - Name of the mask. Without it, the mask is named after the file when it is read. + Boolean masks by mask name and detector bank. Dimensions are named after the + logical dimensions of the bank, and may be a subset of them, in any order. """ with snx.File(filename, 'w') as file: entry = file.create_class('entry', snx.NXentry) - if name is not None: - entry.create_field('title', name) instrument = entry.create_class('instrument', snx.NXinstrument) - for bank, mask in masks.items(): - detector = instrument.create_class(bank, snx.NXdetector) - bits = np.where(mask.values, _USER_DEFINED_MASK, 0) - dataset = detector.create_field( - 'pixel_mask', - sc.array(dims=mask.dims, values=bits, unit=None, dtype='int32'), - ) - for axis, dim in zip(dataset.dims, mask.dims, strict=True): - axis.label = dim + collections = {} + for name, banks in masks.items(): + for bank, mask in banks.items(): + if bank not in collections: + detector = instrument.create_class(bank, snx.NXdetector) + collections[bank] = detector.create_class('masks', snx.NXcollection) + dataset = collections[bank].create_field( + name, mask.values.astype('uint8') + ) + for axis, dim in zip(dataset.dims, mask.dims, strict=True): + axis.label = dim def read_xml_masked_detector_ids(filename: str | Path) -> sc.Variable: @@ -205,15 +198,15 @@ def read_xml_masked_detector_ids(filename: str | Path) -> sc.Variable: ) -def _read_mask_file(filename: Path) -> tuple[str, sc.Variable | sc.DataGroup]: +def _read_mask_file(filename: Path) -> dict[str, sc.Variable | sc.DataGroup]: if filename.suffix == '.xml': - return filename.name, read_xml_masked_detector_ids(filename) + return {filename.name: read_xml_masked_detector_ids(filename)} if filename.suffix in _HDF5_SUFFIXES: return _read_hdf5(filename) raise ValueError(f"Unsupported suffix of mask file '{filename}'.") -def _read_hdf5(filename: Path) -> tuple[str, sc.Variable | sc.DataGroup]: +def _read_hdf5(filename: Path) -> dict[str, sc.Variable | sc.DataGroup]: with snx.File(filename) as file: # Reading only one of several entries would leave pixels unmasked. if len(file[snx.NXentry]) > 1: @@ -222,39 +215,39 @@ def _read_hdf5(filename: Path) -> tuple[str, sc.Variable | sc.DataGroup]: return _read_nexus_masks(file['entry'], filename) ids = sc.io.load_hdf5(filename) if isinstance(ids, sc.Variable) and ids.dtype in ('int32', 'int64'): - return filename.name, ids + return {filename.name: ids} raise ValueError( f"Mask file '{filename}' holds neither a NeXus entry nor integer detector IDs." ) -def _read_nexus_masks(entry: snx.Group, filename: Path) -> tuple[str, sc.DataGroup]: - masks = { - bank: _read_bank_mask(detector, filename) - for bank, detector in entry['instrument'][snx.NXdetector].items() - if 'pixel_mask' in detector - } +def _read_nexus_masks(entry: snx.Group, filename: Path) -> dict[str, sc.DataGroup]: + masks = {} + for bank, detector in entry['instrument'][snx.NXdetector].items(): + if 'masks' not in detector: + continue + for name, field in detector['masks'].items(): + masks.setdefault(name, {})[bank] = _read_bank_mask(field, filename) if not masks: - raise ValueError(f"Mask file '{filename}' has no detector with a pixel_mask.") - name = entry['title'][()] if 'title' in entry else filename.name - return name, sc.DataGroup(masks) - - -def _read_bank_mask(detector: snx.Group, filename: Path) -> sc.Variable: - # NeXus combines these into one mask. They are rejected instead of ignored, since - # ignoring them would leave pixels unmasked. - if numbered := [key for key in detector if re.fullmatch(r'pixel_mask_\d+', key)]: + raise ValueError(f"Mask file '{filename}' has no detector with masks.") + return {name: sc.DataGroup(banks) for name, banks in masks.items()} + + +def _read_bank_mask(field: snx.Field | snx.Group, filename: Path) -> sc.Variable: + if not isinstance(field, snx.Field): + raise ValueError(f"Mask file '{filename}' has a group '{field.name}' in masks.") + dataset = field.dataset + values = dataset[()] + # h5py reads its boolean enumeration as bool. + if not (values.dtype == bool or np.issubdtype(values.dtype, np.integer)): + raise ValueError(f"Mask '{dataset.name}' in '{filename}' is not integer.") + # scippnexus does not read HDF5 dimension labels of fields in an NXcollection. + dims = [axis.label for axis in dataset.dims] + if not all(dims): raise ValueError( - f"Mask file '{filename}' has several masks for one bank: {numbered}." + f"Mask '{dataset.name}' in '{filename}' lacks HDF5 dimension labels." ) - field = detector['pixel_mask'] - # Loading the field as a variable would clip uint32 to int32 and change the bits. - values = field.dataset[()] - if not np.issubdtype(values.dtype, np.integer): - raise ValueError(f"Mask file '{filename}' has a non-integer pixel_mask.") - # int64 holds all bits to test for any input type, unlike e.g. uint8. - masked = values.astype('int64') & _REJECTING_BITS != 0 - return sc.array(dims=field.dims, values=masked) + return sc.array(dims=dims, values=values != 0) def _validate_masked_ids(masked_ids: sc.Variable, name: str) -> sc.Variable: diff --git a/packages/essreduce/tests/masking_test.py b/packages/essreduce/tests/masking_test.py index c54e3f1b6..1b53f82c5 100644 --- a/packages/essreduce/tests/masking_test.py +++ b/packages/essreduce/tests/masking_test.py @@ -2,6 +2,7 @@ # Copyright (c) 2026 Scipp contributors (https://github.com/scipp) from pathlib import Path +import h5py import numpy as np import pytest import scipp as sc @@ -35,14 +36,13 @@ def _read(*paths: Path) -> PixelMasks: return read_pixel_masks(PixelMaskFilenames(paths)) -def _write_raw_pixel_mask( - path: Path, field: str, values: np.ndarray, label: str = 'tube' -) -> Path: +def _write_raw_mask(path: Path, values: np.ndarray, label: str = 'tube') -> Path: with snx.File(path, 'w') as file: entry = file.create_class('entry', snx.NXentry) instrument = entry.create_class('instrument', snx.NXinstrument) detector = instrument.create_class('bank_0', snx.NXdetector) - dataset = detector.create_field(field, values) + collection = detector.create_class('masks', snx.NXcollection) + dataset = collection.create_field('m', values) dataset.dims[0].label = label return path @@ -91,17 +91,16 @@ def test_read_raises_if_hdf5_does_not_hold_integers(tmp_path: Path) -> None: _read(path) -def test_read_bank_masks_names_mask_after_file_by_default(tmp_path: Path) -> None: - mask = sc.array(dims=['tube'], values=[True, False]) - write_pixel_masks(tmp_path / 'a.nxs', {'bank_0': mask}) +def test_read_bank_masks_names_masks_after_datasets(tmp_path: Path) -> None: + broken = sc.array(dims=['tube'], values=[True, False]) + beamstop = sc.array(dims=['straw'], values=[False, True, True]) + write_pixel_masks( + tmp_path / 'a.nxs', + {'broken': {'bank_0': broken}, 'beamstop': {'bank_0': beamstop}}, + ) masks = _read(tmp_path / 'a.nxs') - assert sc.identical(masks['a.nxs']['bank_0'], mask) - - -def test_read_bank_masks_uses_name_stored_in_file(tmp_path: Path) -> None: - mask = sc.array(dims=['tube'], values=[True, False]) - write_pixel_masks(tmp_path / 'a.nxs', {'bank_0': mask}, name='broken') - assert list(_read(tmp_path / 'a.nxs')) == ['broken'] + assert sc.identical(masks['broken'], sc.DataGroup(bank_0=broken)) + assert sc.identical(masks['beamstop'], sc.DataGroup(bank_0=beamstop)) def test_read_bank_masks_keeps_dimensions_of_all_banks(tmp_path: Path) -> None: @@ -109,76 +108,106 @@ def test_read_bank_masks_keeps_dimensions_of_all_banks(tmp_path: Path) -> None: 'bank_0': sc.array(dims=['layer', 'tube'], values=[[True, False]]), 'bank_1': sc.array(dims=['straw'], values=[False, True, False]), } - write_pixel_masks(tmp_path / 'a.nxs', banks) - loaded = _read(tmp_path / 'a.nxs')['a.nxs'] - assert sc.identical(loaded, sc.DataGroup(banks)) + write_pixel_masks(tmp_path / 'a.nxs', {'m': banks}) + assert sc.identical(_read(tmp_path / 'a.nxs')['m'], sc.DataGroup(banks)) + + +def test_read_bank_masks_omits_banks_without_dataset_of_mask(tmp_path: Path) -> None: + mask = sc.array(dims=['tube'], values=[True]) + write_pixel_masks( + tmp_path / 'a.nxs', {'broken': {'bank_0': mask}, 'beamstop': {'bank_1': mask}} + ) + masks = _read(tmp_path / 'a.nxs') + assert list(masks['broken']) == ['bank_0'] + assert list(masks['beamstop']) == ['bank_1'] def test_read_ignores_other_entries_of_the_file(tmp_path: Path) -> None: path = tmp_path / 'a.nxs' - write_pixel_masks(path, {'bank_0': sc.array(dims=['tube'], values=[True])}) + write_pixel_masks(path, {'m': {'bank_0': sc.array(dims=['tube'], values=[True])}}) with snx.File(path, 'r+') as file: file['entry'].create_field('valid_from', '2026-11-01T00:00:00Z') - assert list(_read(path)['a.nxs']) == ['bank_0'] + file['entry/instrument/bank_0'].create_field('pixel_mask', np.array([1])) + assert list(_read(path)) == ['m'] -def test_read_skips_detector_without_pixel_mask(tmp_path: Path) -> None: +def test_read_skips_detector_without_masks(tmp_path: Path) -> None: path = tmp_path / 'a.nxs' - write_pixel_masks(path, {'bank_0': sc.array(dims=['tube'], values=[True])}) + write_pixel_masks(path, {'m': {'bank_0': sc.array(dims=['tube'], values=[True])}}) with snx.File(path, 'r+') as file: file['entry/instrument'].create_class('bank_1', snx.NXdetector) - assert list(_read(path)['a.nxs']) == ['bank_0'] + assert list(_read(path)['m']) == ['bank_0'] + + +def test_read_combines_masks_of_several_files(tmp_path: Path) -> None: + mask = sc.array(dims=['tube'], values=[True]) + write_pixel_masks(tmp_path / 'a.nxs', {'broken': {'bank_0': mask}}) + write_pixel_masks(tmp_path / 'b.nxs', {'beamstop': {'bank_0': mask}}) + masks = _read(tmp_path / 'a.nxs', tmp_path / 'b.nxs') + assert list(masks) == ['broken', 'beamstop'] -def test_read_raises_if_stored_names_are_equal(tmp_path: Path) -> None: +def test_read_raises_if_files_have_mask_with_same_name(tmp_path: Path) -> None: mask = sc.array(dims=['tube'], values=[True]) - write_pixel_masks(tmp_path / 'a.nxs', {'bank_0': mask}, name='broken') - write_pixel_masks(tmp_path / 'b.nxs', {'bank_0': mask}, name='broken') + write_pixel_masks(tmp_path / 'a.nxs', {'broken': {'bank_0': mask}}) + write_pixel_masks(tmp_path / 'b.nxs', {'broken': {'bank_1': mask}}) with pytest.raises(ValueError, match='broken'): _read(tmp_path / 'a.nxs', tmp_path / 'b.nxs') -@pytest.mark.parametrize('dtype', ['uint8', 'int32', 'int64']) -def test_read_masks_pixels_with_any_rejecting_bit(tmp_path: Path, dtype: str) -> None: - values = np.array([0, 1, 2], dtype=dtype) - path = _write_raw_pixel_mask(tmp_path / 'a.nxs', 'pixel_mask', values) - loaded = _read(path)['a.nxs']['bank_0'] +def test_write_stores_masks_as_uint8(tmp_path: Path) -> None: + path = tmp_path / 'a.nxs' + write_pixel_masks(path, {'m': {'bank_0': sc.array(dims=['tube'], values=[True])}}) + with h5py.File(path) as file: + assert file['entry/instrument/bank_0/masks/m'].dtype == 'uint8' + + +@pytest.mark.parametrize('dtype', ['bool', 'uint8', 'int32', 'uint32', 'int64']) +def test_read_masks_pixels_with_nonzero_value(tmp_path: Path, dtype: str) -> None: + values = np.array([0, 1, 1], dtype=dtype) + if dtype != 'bool': + values[2] = 2 + loaded = _read(_write_raw_mask(tmp_path / 'a.nxs', values))['m']['bank_0'] assert loaded.dims == ('tube',) assert loaded.values.tolist() == [False, True, True] -def test_read_does_not_mask_pixels_with_tag_bits_only(tmp_path: Path) -> None: - values = np.array([1 << 8, 1 << 31], dtype='uint32') - path = _write_raw_pixel_mask(tmp_path / 'a.nxs', 'pixel_mask', values) - assert _read(path)['a.nxs']['bank_0'].values.tolist() == [True, False] +def test_read_raises_if_mask_is_not_integer(tmp_path: Path) -> None: + path = _write_raw_mask(tmp_path / 'a.nxs', np.array([1.5])) + with pytest.raises(ValueError, match='not integer'): + _read(path) -def test_read_raises_if_pixel_mask_is_not_integer(tmp_path: Path) -> None: - path = _write_raw_pixel_mask(tmp_path / 'a.nxs', 'pixel_mask', np.array([1.5])) - with pytest.raises(ValueError, match='non-integer'): +def test_read_raises_if_mask_has_no_dimension_labels(tmp_path: Path) -> None: + path = _write_raw_mask(tmp_path / 'a.nxs', np.array([0, 1, 0]), label='') + with pytest.raises(ValueError, match='dimension labels'): _read(path) -def test_read_raises_on_numbered_pixel_masks(tmp_path: Path) -> None: - path = _write_raw_pixel_mask(tmp_path / 'a.nxs', 'pixel_mask', np.array([1])) +def test_read_raises_on_group_in_masks(tmp_path: Path) -> None: + path = _write_raw_mask(tmp_path / 'a.nxs', np.array([1])) with snx.File(path, 'r+') as file: - file['entry/instrument/bank_0'].create_field('pixel_mask_2', np.array([1])) - with pytest.raises(ValueError, match='pixel_mask_2'): + file['entry/instrument/bank_0/masks'].create_class('sub', snx.NXcollection) + with pytest.raises(ValueError, match='group'): _read(path) def test_read_raises_on_several_entries(tmp_path: Path) -> None: path = tmp_path / 'a.nxs' - write_pixel_masks(path, {'bank_0': sc.array(dims=['tube'], values=[True])}) + write_pixel_masks(path, {'m': {'bank_0': sc.array(dims=['tube'], values=[True])}}) with snx.File(path, 'r+') as file: file.create_class('other', snx.NXentry) with pytest.raises(ValueError, match='several NXentry'): _read(path) -def test_read_raises_if_file_has_no_pixel_mask(tmp_path: Path) -> None: - path = _write_raw_pixel_mask(tmp_path / 'a.nxs', 'detector_number', np.array([1])) - with pytest.raises(ValueError, match='no detector with a pixel_mask'): +def test_read_raises_if_file_has_no_masks(tmp_path: Path) -> None: + path = tmp_path / 'a.nxs' + with snx.File(path, 'w') as file: + entry = file.create_class('entry', snx.NXentry) + instrument = entry.create_class('instrument', snx.NXinstrument) + instrument.create_class('bank_0', snx.NXdetector) + with pytest.raises(ValueError, match='no detector with masks'): _read(path) @@ -268,15 +297,6 @@ def test_to_detector_masks_raises_if_mask_does_not_fit_detector( to_detector_masks(_per_bank(a=mask), bank_ids, bank='a') -def test_to_detector_masks_raises_if_file_has_no_dimension_labels( - bank_ids: sc.Variable, tmp_path: Path -) -> None: - values = np.array([0, 1, 0]) - path = _write_raw_pixel_mask(tmp_path / 'a.nxs', 'pixel_mask', values, label='') - with pytest.raises(sc.DimensionError): - to_detector_masks(_read(path), bank_ids, bank='bank_0') - - @pytest.mark.parametrize( ('mask_dims', 'expected_dims'), [ diff --git a/packages/esssans/tests/loki/workflow_test.py b/packages/esssans/tests/loki/workflow_test.py index 6cdf6a348..0f10cfa9e 100644 --- a/packages/esssans/tests/loki/workflow_test.py +++ b/packages/esssans/tests/loki/workflow_test.py @@ -162,7 +162,7 @@ def test_loki_analytical_lookup_table_matches_simulated_source_pulse(): def test_loki_workflow_reads_bool_masks_per_bank(tmp_path): path = tmp_path / 'masks.h5' mask = sc.array(dims=['tube'], values=[True, False]) - write_pixel_masks(path, {'loki_detector_1': mask}, name='broken') + write_pixel_masks(path, {'broken': {'loki_detector_1': mask}}) wf = loki.LokiWorkflow() wf[NeXusDetectorName] = 'loki_detector_1' wf[DetectorIDs] = sc.array(dims=['tube'], values=[1, 2], unit=None) @@ -174,7 +174,7 @@ def test_loki_workflow_reads_bool_masks_per_bank(tmp_path): def test_loki_workflow_raises_if_mask_file_has_unknown_bank(tmp_path): path = tmp_path / 'masks.h5' mask = sc.array(dims=['tube'], values=[True, False]) - write_pixel_masks(path, {'loki_detector_99': mask}) + write_pixel_masks(path, {'broken': {'loki_detector_99': mask}}) wf = loki.LokiWorkflow() wf[NeXusDetectorName] = 'loki_detector_1' wf[DetectorIDs] = sc.array(dims=['tube'], values=[1, 2], unit=None) @@ -216,7 +216,7 @@ def test_bank_mask_gives_same_iofq_as_equivalent_id_file( assert 0 < masked_ids.size < ids.size bank_file = tmp_path / 'bank.nxs' - write_pixel_masks(bank_file, {'larmor_detector': mask}) + write_pixel_masks(bank_file, {'m': {'larmor_detector': mask}}) id_file = tmp_path / 'ids.h5' sc.io.save_hdf5(masked_ids, id_file) @@ -233,7 +233,7 @@ def test_bool_mask_file_gives_same_masks_as_id_file(larmor_workflow, tmp_path): from_ids = next(iter(wf.compute(DetectorMasks).values())) path = tmp_path / 'masks.h5' - write_pixel_masks(path, {'larmor_detector': from_ids}) + write_pixel_masks(path, {'m': {'larmor_detector': from_ids}}) wf[PixelMaskFilenames] = [path] - assert sc.identical(wf.compute(DetectorMasks)['masks.h5'], from_ids) + assert sc.identical(wf.compute(DetectorMasks)['m'], from_ids) From 2ecefa0a7e489101d083bb594d0b3e8192f65c1e Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Tue, 10 Nov 2026 10:14:23 +0000 Subject: [PATCH 12/16] Accept paths as values of multi-filename widgets LokiAtLarmorTutorialWorkflow sets PixelMaskFilenames to a list of paths. Creating its widget failed on str.join, and the parameter box then showed no parameters at all, which broke the LoKI widget notebook. Co-Authored-By: Claude Opus 5.5 --- .../src/ess/reduce/widgets/_string_widget.py | 15 +++++++++++---- packages/essreduce/tests/widget_test.py | 17 ++++++++++++++++- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/packages/essreduce/src/ess/reduce/widgets/_string_widget.py b/packages/essreduce/src/ess/reduce/widgets/_string_widget.py index c91fb70ba..c31e92734 100644 --- a/packages/essreduce/src/ess/reduce/widgets/_string_widget.py +++ b/packages/essreduce/src/ess/reduce/widgets/_string_widget.py @@ -1,5 +1,8 @@ # SPDX-License-Identifier: BSD-3-Clause # Copyright (c) 2024 Scipp contributors (https://github.com/scipp) +from collections.abc import Sequence +from os import PathLike + from ipywidgets import HBox, Text, ValueWidget from ._config import default_layout @@ -28,11 +31,14 @@ def value(self, value: str | None): class MultiStringWidget(StringWidget): def __init__( - self, description: str, value: tuple[str, ...] | None = None, **kwargs + self, + description: str, + value: Sequence[str | PathLike] | str | None = None, + **kwargs, ): # Special case handling to allow initialising with a single string if not isinstance(value, str) and value is not None: - value = ', '.join(value) + value = ', '.join(map(str, value)) super().__init__(description, value, **kwargs) @@ -44,5 +50,6 @@ def value(self) -> tuple[str, ...]: return tuple(s.strip() for s in v.split(',')) @value.setter - def value(self, value: tuple[str, ...]): - self.text_widget.value = ', '.join(value) + def value(self, value: Sequence[str | PathLike]): + # Filenames may be given as paths, such as default values of workflows. + self.text_widget.value = ', '.join(map(str, value)) diff --git a/packages/essreduce/tests/widget_test.py b/packages/essreduce/tests/widget_test.py index b20b57480..4f46ff38c 100644 --- a/packages/essreduce/tests/widget_test.py +++ b/packages/essreduce/tests/widget_test.py @@ -2,6 +2,7 @@ # Copyright (c) 2024 Scipp contributors (https://github.com/scipp) from collections.abc import Callable, Generator from contextlib import contextmanager +from pathlib import Path from typing import Any, NewType import pytest @@ -18,7 +19,12 @@ parameter_registry, ) from ess.reduce.ui import ResultBox, WorkflowWidget, workflow_widget -from ess.reduce.widgets import OptionalWidget, SwitchWidget, create_parameter_widget +from ess.reduce.widgets import ( + MultiFilenameWidget, + OptionalWidget, + SwitchWidget, + create_parameter_widget, +) from ess.reduce.widgets._base import WidgetWithFieldsProtocol, get_fields, set_fields from ess.reduce.workflow import register_workflow, workflow_registry @@ -583,6 +589,15 @@ def test_filename_widget_multiple() -> None: assert param_widget.value == ("file1.hdf", "file2.hdf") +def test_multi_filename_widget_accepts_paths() -> None: + widget = MultiFilenameWidget( + description='files', value=[Path('a.xml'), Path('dir/b.nxs')] + ) + assert widget.value == ('a.xml', str(Path('dir/b.nxs'))) + widget.value = [Path('c.h5')] + assert widget.value == ('c.h5',) + + def test_filename_widget_multiple_initialized_with_single_string() -> None: widget = _ready_widget( providers=[multi_filename_print_provider_bis], output_selections=[list[str]] From de65bacab3c9bdb5ff7f4a7a23377eb3ece0b721 Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Wed, 11 Nov 2026 07:06:57 +0000 Subject: [PATCH 13/16] Pass the detector instead of its IDs to to_detector_masks Masks given per bank match the detector by its logical dimensions and never use detector IDs. Taking only the IDs hid this, and suggested that every mask goes through an ID lookup. The function now takes the detector data array: masks per bank use its sizes, and only masks given as a list of IDs read the ID coordinate. Its name is a parameter, since ISIS data from Mantid and POWGEN data do not use 'detector_number'. The SANS DetectorIDs key existed only to feed masking, so it is removed and the mask providers take EmptyDetector[SampleRun]. Co-Authored-By: Claude Opus 5.5 --- .../essdiffraction/src/ess/powder/masking.py | 2 +- packages/essreduce/src/ess/reduce/masking.py | 38 +++++----- packages/essreduce/tests/masking_test.py | 69 ++++++++++++------- packages/esssans/src/ess/isissans/general.py | 22 ++---- packages/esssans/src/ess/isissans/sans2d.py | 9 ++- packages/esssans/src/ess/sans/masking.py | 20 ++---- packages/esssans/src/ess/sans/types.py | 4 -- .../tests/isissans/sans2d_reduction_test.py | 11 +-- packages/esssans/tests/loki/workflow_test.py | 7 +- 9 files changed, 91 insertions(+), 91 deletions(-) diff --git a/packages/essdiffraction/src/ess/powder/masking.py b/packages/essdiffraction/src/ess/powder/masking.py index 06654212e..2be51c2dc 100644 --- a/packages/essdiffraction/src/ess/powder/masking.py +++ b/packages/essdiffraction/src/ess/powder/masking.py @@ -31,7 +31,7 @@ def apply_masks( key = ( set(out.coords.keys()) & {"detector_number", "detector_id", "spectrum"} ).pop() - out = out.assign_masks(to_detector_masks(pixel_masks, out.coords[key])) + out = out.assign_masks(to_detector_masks(pixel_masks, out, id_coord=key)) for dim, mask in { "tof": tof_mask_func, diff --git a/packages/essreduce/src/ess/reduce/masking.py b/packages/essreduce/src/ess/reduce/masking.py index f7b3939e6..41373ba24 100644 --- a/packages/essreduce/src/ess/reduce/masking.py +++ b/packages/essreduce/src/ess/reduce/masking.py @@ -82,47 +82,49 @@ def read_pixel_masks(filenames: PixelMaskFilenames) -> PixelMasks: def to_detector_masks( pixel_masks: PixelMasks, - ids: sc.Variable, + detector: sc.DataArray, bank: str | None = None, known_banks: Collection[str] | None = None, + id_coord: str = 'detector_number', ) -> dict[str, sc.Variable]: """Create boolean masks for the pixels of a detector bank. The masks use the logical dimensions of the bank, which are the dimensions of - ``ids``: + ``detector``: - Masks given per bank are selected by ``bank``. They keep their dimensions, which - may be a subset of those of ``ids``, and are transposed to the order of ``ids``. + may be a subset of those of ``detector``, and are transposed to the order of + ``detector``. Detector IDs are not used. - Masks given as a list of detector IDs, such as ISIS XML files, apply to any bank, since IDs identify a pixel. They are converted to a mask with the dimensions of - ``ids`` by looking up each pixel's ID in the list. + the ID coordinate of ``detector`` by looking up each pixel's ID in the list. Parameters ---------- pixel_masks: Pixel masks of the instrument. - ids: - Detector IDs of the pixels. + detector: + Data of the detector bank, with its logical dimensions. bank: Name of the detector bank. Without it, masks given per bank raise an error. known_banks: Names of all banks of the instrument. If given, a file with masks for any other bank raises an error. A misspelled name would otherwise mask nothing. + id_coord: + Name of the coordinate of ``detector`` that holds the detector IDs. Only masks + given as a list of detector IDs use it. """ masks = {} for name, content in pixel_masks.items(): if isinstance(content, sc.Variable): - masked_ids = _validate_masked_ids(content, name) - masks[name] = sc.array( - dims=ids.dims, values=np.isin(ids.values, masked_ids.values) - ) + masks[name] = _mask_from_ids(content, detector.coords[id_coord], name) continue if bank is None: raise ValueError(f"Mask '{name}' is given per bank, but no bank is known.") if known_banks is not None and (unknown := set(content) - set(known_banks)): raise ValueError(f"Mask '{name}' has unknown banks: {sorted(unknown)}.") if bank in content: - masks[name] = _align_bank_mask(content[bank], ids, name) + masks[name] = _align_bank_mask(content[bank], detector.sizes, name) return masks @@ -250,22 +252,24 @@ def _read_bank_mask(field: snx.Field | snx.Group, filename: Path) -> sc.Variable return sc.array(dims=dims, values=values != 0) -def _validate_masked_ids(masked_ids: sc.Variable, name: str) -> sc.Variable: +def _mask_from_ids(masked_ids: sc.Variable, ids: sc.Variable, name: str) -> sc.Variable: if masked_ids.dtype not in ('int32', 'int64'): raise sc.DTypeError( f"Mask '{name}' must hold integer detector IDs, not {masked_ids.dtype}." ) - return masked_ids + return sc.array(dims=ids.dims, values=np.isin(ids.values, masked_ids.values)) -def _align_bank_mask(mask: sc.Variable, ids: sc.Variable, name: str) -> sc.Variable: +def _align_bank_mask( + mask: sc.Variable, sizes: Mapping[str, int], name: str +) -> sc.Variable: if mask.dtype != 'bool': raise sc.DTypeError(f"Mask '{name}' must be boolean, not {mask.dtype}.") # Masks with a dimension that the data lacks would be ignored without an error. - if not mask.sizes.items() <= ids.sizes.items(): + if not mask.sizes.items() <= sizes.items(): raise sc.DimensionError( f"Mask '{name}' has sizes {mask.sizes}, which are not part of the sizes " - f"of the detector, {ids.sizes}." + f"of the detector, {sizes}." ) # A mask file may use any order. DetectorMasks then has one layout for all of them. - return mask.transpose([dim for dim in ids.dims if dim in mask.dims]) + return mask.transpose([dim for dim in sizes if dim in mask.dims]) diff --git a/packages/essreduce/tests/masking_test.py b/packages/essreduce/tests/masking_test.py index 1b53f82c5..6808268f2 100644 --- a/packages/essreduce/tests/masking_test.py +++ b/packages/essreduce/tests/masking_test.py @@ -211,17 +211,40 @@ def test_read_raises_if_file_has_no_masks(tmp_path: Path) -> None: _read(path) +def _detector(ids: sc.Variable, id_coord: str = 'detector_number') -> sc.DataArray: + return sc.DataArray(sc.zeros(sizes=ids.sizes), coords={id_coord: ids}) + + def test_to_detector_masks_keeps_shape_of_ids() -> None: ids = sc.array(dims=['tube', 'pixel'], values=[[1, 2], [3, 4]], unit=None) masked = sc.array(dims=['detector_id'], values=[2, 3, 99], unit=None) - masks = to_detector_masks(PixelMasks({'m': masked}), ids) + masks = to_detector_masks(PixelMasks({'m': masked}), _detector(ids)) expected = sc.array(dims=['tube', 'pixel'], values=[[False, True], [True, False]]) assert sc.identical(masks['m'], expected) +def test_to_detector_masks_reads_ids_from_given_coord() -> None: + ids = sc.array(dims=['spectrum'], values=[1, 2, 3], unit=None) + masked = sc.array(dims=['detector_id'], values=[2], unit=None) + masks = to_detector_masks( + PixelMasks({'m': masked}), _detector(ids, 'detector_id'), id_coord='detector_id' + ) + assert sc.identical( + masks['m'], sc.array(dims=['spectrum'], values=[False, True, False]) + ) + + +def test_to_detector_masks_does_not_need_ids_for_masks_per_bank() -> None: + detector = sc.DataArray(sc.zeros(sizes={'tube': 3, 'pixel': 2})) + mask = sc.array(dims=['tube'], values=[True, False, False]) + masks = to_detector_masks(_per_bank(a=mask), detector, bank='a') + assert sc.identical(masks['m'], mask) + + @pytest.fixture -def bank_ids() -> sc.Variable: - return sc.array(dims=['tube', 'pixel'], values=[[1, 2], [3, 4], [5, 6]], unit=None) +def bank() -> sc.DataArray: + ids = sc.array(dims=['tube', 'pixel'], values=[[1, 2], [3, 4], [5, 6]], unit=None) + return _detector(ids) def _per_bank(**banks: sc.Variable) -> PixelMasks: @@ -229,55 +252,55 @@ def _per_bank(**banks: sc.Variable) -> PixelMasks: def test_to_detector_masks_selects_bank_and_allows_fewer_dims( - bank_ids: sc.Variable, + bank: sc.DataArray, ) -> None: mask = sc.array(dims=['tube'], values=[True, False, False]) other = sc.array(dims=['tube'], values=[False, True, False]) - masks = to_detector_masks(_per_bank(a=mask, b=other), bank_ids, bank='a') + masks = to_detector_masks(_per_bank(a=mask, b=other), bank, bank='a') assert sc.identical(masks['m'], mask) -def test_to_detector_masks_skips_file_without_this_bank(bank_ids: sc.Variable) -> None: +def test_to_detector_masks_skips_file_without_this_bank(bank: sc.DataArray) -> None: mask = sc.array(dims=['tube'], values=[True, False, False]) - assert to_detector_masks(_per_bank(b=mask), bank_ids, bank='a') == {} + assert to_detector_masks(_per_bank(b=mask), bank, bank='a') == {} -def test_to_detector_masks_applies_ids_to_any_bank(bank_ids: sc.Variable) -> None: +def test_to_detector_masks_applies_ids_to_any_bank(bank: sc.DataArray) -> None: masked = sc.array(dims=['detector_id'], values=[1], unit=None) - masks = to_detector_masks(PixelMasks({'m': masked}), bank_ids, bank='a') + masks = to_detector_masks(PixelMasks({'m': masked}), bank, bank='a') assert masks['m'].values.sum() == 1 def test_to_detector_masks_raises_without_bank_for_masks_per_bank( - bank_ids: sc.Variable, + bank: sc.DataArray, ) -> None: mask = sc.array(dims=['tube'], values=[True, False, False]) with pytest.raises(ValueError, match='no bank is known'): - to_detector_masks(_per_bank(a=mask), bank_ids) + to_detector_masks(_per_bank(a=mask), bank) -def test_to_detector_masks_raises_on_unknown_bank(bank_ids: sc.Variable) -> None: +def test_to_detector_masks_raises_on_unknown_bank(bank: sc.DataArray) -> None: mask = sc.array(dims=['tube'], values=[True, False, False]) with pytest.raises(ValueError, match='typo'): to_detector_masks( - _per_bank(a=mask, typo=mask), bank_ids, bank='a', known_banks={'a', 'b'} + _per_bank(a=mask, typo=mask), bank, bank='a', known_banks={'a', 'b'} ) def test_to_detector_masks_raises_if_masked_ids_are_not_integer( - bank_ids: sc.Variable, + bank: sc.DataArray, ) -> None: masked = sc.array(dims=['x'], values=[True]) with pytest.raises(sc.DTypeError, match='integer detector IDs'): - to_detector_masks(PixelMasks({'m': masked}), bank_ids) + to_detector_masks(PixelMasks({'m': masked}), bank) def test_to_detector_masks_raises_if_bank_mask_is_not_boolean( - bank_ids: sc.Variable, + bank: sc.DataArray, ) -> None: mask = sc.array(dims=['tube'], values=[0, 1, 2], unit=None) with pytest.raises(sc.DTypeError, match='boolean'): - to_detector_masks(_per_bank(a=mask), bank_ids, bank='a') + to_detector_masks(_per_bank(a=mask), bank, bank='a') @pytest.mark.parametrize( @@ -291,10 +314,10 @@ def test_to_detector_masks_raises_if_bank_mask_is_not_boolean( ids=['unknown-dim', 'wrong-size', 'sizes-of-other-dim', 'extra-dim'], ) def test_to_detector_masks_raises_if_mask_does_not_fit_detector( - bank_ids: sc.Variable, mask: sc.Variable + bank: sc.DataArray, mask: sc.Variable ) -> None: with pytest.raises(sc.DimensionError): - to_detector_masks(_per_bank(a=mask), bank_ids, bank='a') + to_detector_masks(_per_bank(a=mask), bank, bank='a') @pytest.mark.parametrize( @@ -322,17 +345,17 @@ def test_to_detector_masks_follows_dimension_order_of_detector( mask_dims: tuple[str, ...], expected_dims: tuple[str, ...] ) -> None: sizes = {'layer': 2, 'tube': 3, 'straw': 4} - ids = sc.array(dims=list(sizes), values=np.arange(24).reshape(2, 3, 4), unit=None) + detector = sc.DataArray(sc.zeros(sizes=sizes)) shape = [sizes[dim] for dim in mask_dims] values = np.arange(np.prod(shape)).reshape(shape) % 3 == 0 mask = sc.array(dims=list(mask_dims), values=values) - result = to_detector_masks(_per_bank(a=mask), ids, bank='a')['m'] + result = to_detector_masks(_per_bank(a=mask), detector, bank='a')['m'] assert result.dims == expected_dims assert sc.identical(result, mask.transpose(list(expected_dims))) def test_to_detector_masks_accepts_scalar_for_whole_bank( - bank_ids: sc.Variable, + bank: sc.DataArray, ) -> None: - result = to_detector_masks(_per_bank(a=sc.scalar(True)), bank_ids, bank='a')['m'] + result = to_detector_masks(_per_bank(a=sc.scalar(True)), bank, bank='a')['m'] assert sc.identical(result, sc.scalar(True)) diff --git a/packages/esssans/src/ess/isissans/general.py b/packages/esssans/src/ess/isissans/general.py index fd4f95dfc..98e984d83 100644 --- a/packages/esssans/src/ess/isissans/general.py +++ b/packages/esssans/src/ess/isissans/general.py @@ -12,7 +12,6 @@ import scippneutron as scn import scippnexus as snx from ess.sans.types import ( - DetectorIDs, DetectorMasks, DetectorPixelShape, DetectorPositionOffset, @@ -290,27 +289,15 @@ def lab_frame_transform() -> NeXusTransformation[snx.NXdetector, RunType]: ) -def get_detector_ids_from_sample_run(data: RawDetector[SampleRun]) -> DetectorIDs: - """Extract detector IDs from sample run. - - This overrides the function in the masking module which gets the detector IDs from - the detector before loading event data. In this ISIS case files are loaded using - Mantid which does not load event separately, so we get IDs from the data. - """ - return DetectorIDs( - data.coords[ - 'detector_number' if 'detector_number' in data.coords else 'detector_id' - ] - ) - - -def to_detector_mask_without_bank(ids: DetectorIDs, masks: PixelMasks) -> DetectorMasks: +def to_detector_mask_without_bank( + detector: EmptyDetector[SampleRun], masks: PixelMasks +) -> DetectorMasks: """Create detector masks from the pixel masks of the instrument. This overrides the function in the masking module, since files loaded using Mantid have no NeXus detector name. So mask files that give masks per bank are not usable. """ - return DetectorMasks(to_detector_masks(masks, ids)) + return DetectorMasks(to_detector_masks(masks, detector, id_coord='detector_id')) providers = ( @@ -324,7 +311,6 @@ def to_detector_mask_without_bank(ids: DetectorIDs, masks: PixelMasks) -> Detect get_detector_data, get_calibrated_isis_detector, get_calibrated_isis_monitor, - get_detector_ids_from_sample_run, to_detector_mask_without_bank, get_monitor_data, data_to_wavelength, diff --git a/packages/esssans/src/ess/isissans/sans2d.py b/packages/esssans/src/ess/isissans/sans2d.py index 310aa0c8a..f58aa0672 100644 --- a/packages/esssans/src/ess/isissans/sans2d.py +++ b/packages/esssans/src/ess/isissans/sans2d.py @@ -7,7 +7,6 @@ from ess.sans import SansWorkflow from ess.sans.parameters import typical_outputs from ess.sans.types import ( - DetectorIDs, DetectorMasks, EmptyDetector, PixelMaskFilenames, @@ -57,7 +56,7 @@ def sample_holder_mask( def to_detector_masks( - ids: DetectorIDs, + detector: EmptyDetector[SampleRun], pixel_masks: PixelMasks, edge_mask: DetectorEdgeMask, holder_mask: SampleHolderMask, @@ -66,8 +65,8 @@ def to_detector_masks( Parameters ---------- - ids: - Array of detector IDs. + detector: + Detector of the sample run. pixel_masks: Pixel masks of the instrument. edge_mask: @@ -80,7 +79,7 @@ def to_detector_masks( ValueError If a pixel mask has the name of a computed mask, which would replace it. """ - masks = to_detector_mask_without_bank(ids, pixel_masks) + masks = to_detector_mask_without_bank(detector, pixel_masks) computed = {'edges': edge_mask, 'holder_mask': holder_mask} computed = {name: mask for name, mask in computed.items() if mask is not None} if clashes := masks.keys() & computed.keys(): diff --git a/packages/esssans/src/ess/sans/masking.py b/packages/esssans/src/ess/sans/masking.py index fa8f3bbf9..79b08c575 100644 --- a/packages/esssans/src/ess/sans/masking.py +++ b/packages/esssans/src/ess/sans/masking.py @@ -9,7 +9,6 @@ from .types import ( CorrectedDetector, DetectorBankSizes, - DetectorIDs, DetectorMasks, EmptyDetector, NeXusDetectorName, @@ -21,17 +20,8 @@ ) -def get_detector_ids_from_detector(data: EmptyDetector[SampleRun]) -> DetectorIDs: - """Extract detector IDs from a detector.""" - return DetectorIDs( - data.coords[ - 'detector_number' if 'detector_number' in data.coords else 'detector_id' - ] - ) - - def to_detector_mask( - ids: DetectorIDs, + detector: EmptyDetector[SampleRun], masks: PixelMasks, bank: NeXusDetectorName, bank_sizes: DetectorBankSizes, @@ -40,8 +30,9 @@ def to_detector_mask( Parameters ---------- - ids: - Array of detector IDs + detector: + Detector of the sample run, which gives the logical dimensions and detector IDs + of the bank. masks: Pixel masks of the instrument. bank: @@ -50,7 +41,7 @@ def to_detector_mask( Sizes of the banks of the instrument, which define the valid bank names. """ return DetectorMasks( - to_detector_masks(masks, ids, bank=bank, known_banks=bank_sizes) + to_detector_masks(masks, detector, bank=bank, known_banks=bank_sizes) ) @@ -71,7 +62,6 @@ def apply_pixel_masks( providers = ( - get_detector_ids_from_detector, read_pixel_masks, to_detector_mask, apply_pixel_masks, diff --git a/packages/esssans/src/ess/sans/types.py b/packages/esssans/src/ess/sans/types.py index 9c0efbe8c..ee28d60a2 100644 --- a/packages/esssans/src/ess/sans/types.py +++ b/packages/esssans/src/ess/sans/types.py @@ -184,10 +184,6 @@ class TransmissionRun(Generic[ScatteringRunType]): PixelMaskFilenames = masking_t.PixelMaskFilenames PixelMasks = masking_t.PixelMasks -DetectorIDs = NewType('DetectorIDs', sc.Variable) -"""1-D variable listing all detector IDs.""" - - DetectorMasks = NewType('DetectorMasks', dict[str, sc.Variable]) """Masks for detector pixels""" diff --git a/packages/esssans/tests/isissans/sans2d_reduction_test.py b/packages/esssans/tests/isissans/sans2d_reduction_test.py index 25cf63e99..dfe2343d2 100644 --- a/packages/esssans/tests/isissans/sans2d_reduction_test.py +++ b/packages/esssans/tests/isissans/sans2d_reduction_test.py @@ -15,7 +15,6 @@ BeamCenter, CorrectedDetector, CorrectForGravity, - DetectorIDs, DetectorMasks, DimsToKeep, DirectBeam, @@ -181,7 +180,8 @@ def test_detector_masks_default_to_computed_masks_only(pipeline): def test_mask_files_are_combined_with_computed_masks(pipeline, tmp_path): - detector_id = pipeline.compute(DetectorIDs).values[0] + detector = pipeline.compute(EmptyDetector[SampleRun]) + detector_id = detector.coords['detector_id'].values[0] path = tmp_path / 'broken.xml' path.write_text( '\n' @@ -194,12 +194,15 @@ def test_mask_files_are_combined_with_computed_masks(pipeline, tmp_path): def test_pixel_mask_with_name_of_computed_mask_raises(): - ids = sc.array(dims=['spectrum'], values=[1, 2], unit=None) + detector = sc.DataArray( + sc.zeros(sizes={'spectrum': 2}), + coords={'detector_id': sc.array(dims=['spectrum'], values=[1, 2], unit=None)}, + ) edge_mask = sc.array(dims=['spectrum'], values=[True, False]) masked = sc.array(dims=['detector_id'], values=[1], unit=None) with pytest.raises(ValueError, match='edges'): sans2d.to_detector_masks( - DetectorIDs(ids), + EmptyDetector[SampleRun](detector), PixelMasks({'edges': masked}), sans2d.DetectorEdgeMask(edge_mask), sans2d.SampleHolderMask(None), diff --git a/packages/esssans/tests/loki/workflow_test.py b/packages/esssans/tests/loki/workflow_test.py index 0f10cfa9e..c16b8b04a 100644 --- a/packages/esssans/tests/loki/workflow_test.py +++ b/packages/esssans/tests/loki/workflow_test.py @@ -13,7 +13,6 @@ BackgroundRun, BackgroundSubtractedIofQ, BeamCenter, - DetectorIDs, DetectorMasks, EmptyDetector, Filename, @@ -165,7 +164,7 @@ def test_loki_workflow_reads_bool_masks_per_bank(tmp_path): write_pixel_masks(path, {'broken': {'loki_detector_1': mask}}) wf = loki.LokiWorkflow() wf[NeXusDetectorName] = 'loki_detector_1' - wf[DetectorIDs] = sc.array(dims=['tube'], values=[1, 2], unit=None) + wf[EmptyDetector[SampleRun]] = sc.DataArray(sc.zeros(sizes={'tube': 2})) wf[PixelMaskFilenames] = [path] assert sc.identical(wf.compute(DetectorMasks)['broken'], mask) @@ -177,7 +176,7 @@ def test_loki_workflow_raises_if_mask_file_has_unknown_bank(tmp_path): write_pixel_masks(path, {'broken': {'loki_detector_99': mask}}) wf = loki.LokiWorkflow() wf[NeXusDetectorName] = 'loki_detector_1' - wf[DetectorIDs] = sc.array(dims=['tube'], values=[1, 2], unit=None) + wf[EmptyDetector[SampleRun]] = sc.DataArray(sc.zeros(sizes={'tube': 2})) wf[PixelMaskFilenames] = [path] with pytest.raises(ValueError, match='loki_detector_99'): @@ -200,7 +199,7 @@ def test_bank_mask_gives_same_iofq_as_equivalent_id_file( wf[UncertaintyBroadcastMode] = UncertaintyBroadcastMode.drop # For simplicity, insert a fake beam center instead of computing it. wf[BeamCenter] = sc.vector([0.0, 0.0, 0.0], unit='m') - ids = wf.compute(DetectorIDs) + ids = wf.compute(EmptyDetector[SampleRun]).coords['detector_number'] shape = [ids.sizes[dim] for dim in dims] mask = sc.array( From 6f9e2aba5354724c2c77c1cb9b98e1badb735c9d Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Wed, 11 Nov 2026 07:15:36 +0000 Subject: [PATCH 14/16] Clarify masks across banks and why mask files use uint8 Co-Authored-By: Claude Opus 5.5 --- .../developer/adr/0001-pixel-mask-file-format.md | 7 +++++-- packages/essreduce/src/ess/reduce/masking.py | 13 +++++++------ 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md b/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md index d0c206ed7..6b6af3b2e 100644 --- a/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md +++ b/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md @@ -44,10 +44,13 @@ with h5py.File(path) as f: - Names of `NXdetector` groups are the names of the groups in the NeXus files of the runs. A name that is not a bank of the instrument is an error, since a misspelled name would otherwise mask nothing. - The name of a dataset in `masks` is the name of the mask in the reduced data. - Datasets with the same name in several banks are one mask. + Datasets with the same name in several banks form one mask. + The data of each bank gets a mask of that name, made from the dataset of that bank. A bank without a dataset of that name has no pixels masked by it. - A pixel is masked if its value is not zero. - `ess.reduce.masking.write_pixel_masks` writes `uint8`. + HDF5 has no boolean type. + h5py stores numpy booleans as an enumeration based on `int8`, which other HDF5 libraries may not read as booleans. + `ess.reduce.masking.write_pixel_masks` therefore writes `uint8`, the smallest plain integer type. Readers accept any integer type, and the boolean enumeration that h5py writes for numpy booleans. - Dimensions of a mask are named with HDF5 dimension labels. They are a subset of the logical dimensions of the bank, in any order, with matching sizes. diff --git a/packages/essreduce/src/ess/reduce/masking.py b/packages/essreduce/src/ess/reduce/masking.py index 41373ba24..efe1d2000 100644 --- a/packages/essreduce/src/ess/reduce/masking.py +++ b/packages/essreduce/src/ess/reduce/masking.py @@ -50,12 +50,13 @@ def read_pixel_masks(filenames: PixelMaskFilenames) -> PixelMasks: beamstop integer array Bank names are the names of the NeXus detector groups, and mask names are the - names of the datasets. A mask with the same name in several banks is one mask. - A bank without a dataset of that name has no pixels masked by it. A pixel is - masked if its value is not zero. The dimensions of a dataset are given by HDF5 - dimension labels. They are named after the logical dimensions of the bank, and - may be a subset of them, in any order. Other entries of the file, such as a - validity date, are ignored. + names of the datasets. Datasets with the same name in several banks form one + mask. The data of each bank gets a mask of that name, made from the dataset of + that bank. A bank without a dataset of that name has no pixels masked by it. A + pixel is masked if its value is not zero. The dimensions of a dataset are given + by HDF5 dimension labels. They are named after the logical dimensions of the + bank, and may be a subset of them, in any order. Other entries of the file, such + as a validity date, are ignored. - A file written by :py:func:`scipp.io.save_hdf5` with a variable of masked detector IDs. The mask is named after the file, without the directory, since directories differ between hosts. The same applies to ``.xml`` files. From 229f99c175c4ec768e80774f258c3fe61b5ad10d Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Wed, 11 Nov 2026 11:41:34 +0000 Subject: [PATCH 15/16] Record instrument, creator and file time in mask files write_pixel_masks takes the instrument name and writes it to /entry/instrument/name, as in raw data files, since bank names do not always identify the instrument. It also writes the NeXus root attributes file_time and creator, so a file without a validity date still shows roughly when it was made. Workflows do not read these. Co-Authored-By: Claude Opus 5.5 --- .../adr/0001-pixel-mask-file-format.md | 6 ++ packages/essreduce/src/ess/reduce/masking.py | 30 ++++++++-- packages/essreduce/tests/masking_test.py | 59 +++++++++++++++---- packages/esssans/tests/loki/workflow_test.py | 8 +-- 4 files changed, 83 insertions(+), 20 deletions(-) diff --git a/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md b/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md index 6b6af3b2e..d2f5d0256 100644 --- a/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md +++ b/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md @@ -20,9 +20,11 @@ The planned masking tool for LoKI works on tubes and straws of a bank, after the A mask file is an HDF5 file with the group structure of NeXus: ```text +/ attributes file_time and creator /entry NXentry valid_from "2026-11-01T00:00:00Z", not read by workflows instrument NXinstrument + name "LOKI" loki_detector_0 NXdetector masks NXcollection broken_tubes uint8, HDF5 dimension labels ("tube",) @@ -57,6 +59,10 @@ with h5py.File(path) as f: Each mask has its own dimensions, so a tube mask and a straw mask need not be combined into one array. The sizes are checked when masks are applied, because scipp ignores a mask with a dimension that the data lacks, without an error. - A file with several `NXentry` groups is an error, since the reader would use only one of them and leave pixels unmasked. +- As in raw data files, `/entry/instrument/name` holds the name of the instrument, and the NeXus root attributes `file_time` and `creator` hold the time and the software that wrote the file. + Bank names alone do not always make the instrument obvious. + `file_time` gives a rough idea of whether a file may still be valid, also when it has no `valid_from`. + `write_pixel_masks` writes all three, and workflows do not read them. Workflows take a list of filenames and use the masks of all files. Two files with a mask of the same name are an error. diff --git a/packages/essreduce/src/ess/reduce/masking.py b/packages/essreduce/src/ess/reduce/masking.py index efe1d2000..17ef5b889 100644 --- a/packages/essreduce/src/ess/reduce/masking.py +++ b/packages/essreduce/src/ess/reduce/masking.py @@ -7,13 +7,17 @@ """ from collections.abc import Collection, Mapping, Sequence +from datetime import UTC, datetime from pathlib import Path from typing import NewType +import h5py import numpy as np import scipp as sc import scippnexus as snx +from . import __version__ + PixelMaskFilenames = NewType('PixelMaskFilenames', Sequence[str | Path]) """Files with pixel masks to apply. Use an empty sequence to apply none.""" @@ -44,6 +48,7 @@ def read_pixel_masks(filenames: PixelMaskFilenames) -> PixelMasks: /entry (NXentry) instrument (NXinstrument) + name "LOKI" loki_detector_0 (NXdetector) masks (NXcollection) broken_tubes integer array @@ -56,7 +61,7 @@ def read_pixel_masks(filenames: PixelMaskFilenames) -> PixelMasks: pixel is masked if its value is not zero. The dimensions of a dataset are given by HDF5 dimension labels. They are named after the logical dimensions of the bank, and may be a subset of them, in any order. Other entries of the file, such - as a validity date, are ignored. + as the instrument name or a validity date, are ignored. - A file written by :py:func:`scipp.io.save_hdf5` with a variable of masked detector IDs. The mask is named after the file, without the directory, since directories differ between hosts. The same applies to ``.xml`` files. @@ -130,13 +135,20 @@ def to_detector_masks( def write_pixel_masks( - filename: str | Path, masks: Mapping[str, Mapping[str, sc.Variable]] + filename: str | Path, + masks: Mapping[str, Mapping[str, sc.Variable]], + *, + instrument: str, ) -> None: """Write named boolean masks by detector bank to a NeXus file. See :py:func:`read_pixel_masks` for the layout of the file. Masks are stored as unsigned 8-bit integers, since HDF5 has no boolean type. + As in raw data files, the file records the instrument name in + ``/entry/instrument/name``, and the time and software that wrote it in the root + attributes ``file_time`` and ``creator``. Workflows do not read them. + Parameters ---------- filename: @@ -144,15 +156,21 @@ def write_pixel_masks( masks: Boolean masks by mask name and detector bank. Dimensions are named after the logical dimensions of the bank, and may be a subset of them, in any order. + instrument: + Name of the instrument, as in ``/entry/instrument/name`` of its raw data + files, for example ``'LOKI'``. """ - with snx.File(filename, 'w') as file: - entry = file.create_class('entry', snx.NXentry) - instrument = entry.create_class('instrument', snx.NXinstrument) + with h5py.File(filename, 'w') as h5file: + h5file.attrs['file_time'] = datetime.now(UTC).isoformat(timespec='seconds') + h5file.attrs['creator'] = f'essreduce {__version__}' + entry = snx.File(h5file).create_class('entry', snx.NXentry) + nx_instrument = entry.create_class('instrument', snx.NXinstrument) + nx_instrument.create_field('name', instrument) collections = {} for name, banks in masks.items(): for bank, mask in banks.items(): if bank not in collections: - detector = instrument.create_class(bank, snx.NXdetector) + detector = nx_instrument.create_class(bank, snx.NXdetector) collections[bank] = detector.create_class('masks', snx.NXcollection) dataset = collections[bank].create_field( name, mask.values.astype('uint8') diff --git a/packages/essreduce/tests/masking_test.py b/packages/essreduce/tests/masking_test.py index 6808268f2..4038f52d9 100644 --- a/packages/essreduce/tests/masking_test.py +++ b/packages/essreduce/tests/masking_test.py @@ -1,5 +1,6 @@ # SPDX-License-Identifier: BSD-3-Clause # Copyright (c) 2026 Scipp contributors (https://github.com/scipp) +from datetime import UTC, datetime from pathlib import Path import h5py @@ -97,6 +98,7 @@ def test_read_bank_masks_names_masks_after_datasets(tmp_path: Path) -> None: write_pixel_masks( tmp_path / 'a.nxs', {'broken': {'bank_0': broken}, 'beamstop': {'bank_0': beamstop}}, + instrument='TEST', ) masks = _read(tmp_path / 'a.nxs') assert sc.identical(masks['broken'], sc.DataGroup(bank_0=broken)) @@ -108,14 +110,16 @@ def test_read_bank_masks_keeps_dimensions_of_all_banks(tmp_path: Path) -> None: 'bank_0': sc.array(dims=['layer', 'tube'], values=[[True, False]]), 'bank_1': sc.array(dims=['straw'], values=[False, True, False]), } - write_pixel_masks(tmp_path / 'a.nxs', {'m': banks}) + write_pixel_masks(tmp_path / 'a.nxs', {'m': banks}, instrument='TEST') assert sc.identical(_read(tmp_path / 'a.nxs')['m'], sc.DataGroup(banks)) def test_read_bank_masks_omits_banks_without_dataset_of_mask(tmp_path: Path) -> None: mask = sc.array(dims=['tube'], values=[True]) write_pixel_masks( - tmp_path / 'a.nxs', {'broken': {'bank_0': mask}, 'beamstop': {'bank_1': mask}} + tmp_path / 'a.nxs', + {'broken': {'bank_0': mask}, 'beamstop': {'bank_1': mask}}, + instrument='TEST', ) masks = _read(tmp_path / 'a.nxs') assert list(masks['broken']) == ['bank_0'] @@ -124,7 +128,11 @@ def test_read_bank_masks_omits_banks_without_dataset_of_mask(tmp_path: Path) -> def test_read_ignores_other_entries_of_the_file(tmp_path: Path) -> None: path = tmp_path / 'a.nxs' - write_pixel_masks(path, {'m': {'bank_0': sc.array(dims=['tube'], values=[True])}}) + write_pixel_masks( + path, + {'m': {'bank_0': sc.array(dims=['tube'], values=[True])}}, + instrument='TEST', + ) with snx.File(path, 'r+') as file: file['entry'].create_field('valid_from', '2026-11-01T00:00:00Z') file['entry/instrument/bank_0'].create_field('pixel_mask', np.array([1])) @@ -133,7 +141,11 @@ def test_read_ignores_other_entries_of_the_file(tmp_path: Path) -> None: def test_read_skips_detector_without_masks(tmp_path: Path) -> None: path = tmp_path / 'a.nxs' - write_pixel_masks(path, {'m': {'bank_0': sc.array(dims=['tube'], values=[True])}}) + write_pixel_masks( + path, + {'m': {'bank_0': sc.array(dims=['tube'], values=[True])}}, + instrument='TEST', + ) with snx.File(path, 'r+') as file: file['entry/instrument'].create_class('bank_1', snx.NXdetector) assert list(_read(path)['m']) == ['bank_0'] @@ -141,27 +153,50 @@ def test_read_skips_detector_without_masks(tmp_path: Path) -> None: def test_read_combines_masks_of_several_files(tmp_path: Path) -> None: mask = sc.array(dims=['tube'], values=[True]) - write_pixel_masks(tmp_path / 'a.nxs', {'broken': {'bank_0': mask}}) - write_pixel_masks(tmp_path / 'b.nxs', {'beamstop': {'bank_0': mask}}) + write_pixel_masks( + tmp_path / 'a.nxs', {'broken': {'bank_0': mask}}, instrument='TEST' + ) + write_pixel_masks( + tmp_path / 'b.nxs', {'beamstop': {'bank_0': mask}}, instrument='TEST' + ) masks = _read(tmp_path / 'a.nxs', tmp_path / 'b.nxs') assert list(masks) == ['broken', 'beamstop'] def test_read_raises_if_files_have_mask_with_same_name(tmp_path: Path) -> None: mask = sc.array(dims=['tube'], values=[True]) - write_pixel_masks(tmp_path / 'a.nxs', {'broken': {'bank_0': mask}}) - write_pixel_masks(tmp_path / 'b.nxs', {'broken': {'bank_1': mask}}) + write_pixel_masks( + tmp_path / 'a.nxs', {'broken': {'bank_0': mask}}, instrument='TEST' + ) + write_pixel_masks( + tmp_path / 'b.nxs', {'broken': {'bank_1': mask}}, instrument='TEST' + ) with pytest.raises(ValueError, match='broken'): _read(tmp_path / 'a.nxs', tmp_path / 'b.nxs') def test_write_stores_masks_as_uint8(tmp_path: Path) -> None: path = tmp_path / 'a.nxs' - write_pixel_masks(path, {'m': {'bank_0': sc.array(dims=['tube'], values=[True])}}) + write_pixel_masks( + path, + {'m': {'bank_0': sc.array(dims=['tube'], values=[True])}}, + instrument='TEST', + ) with h5py.File(path) as file: assert file['entry/instrument/bank_0/masks/m'].dtype == 'uint8' +def test_write_records_instrument_creator_and_time(tmp_path: Path) -> None: + path = tmp_path / 'a.nxs' + mask = sc.array(dims=['tube'], values=[True]) + before = datetime.now(UTC).replace(microsecond=0) + write_pixel_masks(path, {'m': {'bank_0': mask}}, instrument='LOKI') + with h5py.File(path) as file: + assert file['entry/instrument/name'][()] == b'LOKI' + assert file.attrs['creator'].startswith('essreduce ') + assert before <= datetime.fromisoformat(file.attrs['file_time']) + + @pytest.mark.parametrize('dtype', ['bool', 'uint8', 'int32', 'uint32', 'int64']) def test_read_masks_pixels_with_nonzero_value(tmp_path: Path, dtype: str) -> None: values = np.array([0, 1, 1], dtype=dtype) @@ -194,7 +229,11 @@ def test_read_raises_on_group_in_masks(tmp_path: Path) -> None: def test_read_raises_on_several_entries(tmp_path: Path) -> None: path = tmp_path / 'a.nxs' - write_pixel_masks(path, {'m': {'bank_0': sc.array(dims=['tube'], values=[True])}}) + write_pixel_masks( + path, + {'m': {'bank_0': sc.array(dims=['tube'], values=[True])}}, + instrument='TEST', + ) with snx.File(path, 'r+') as file: file.create_class('other', snx.NXentry) with pytest.raises(ValueError, match='several NXentry'): diff --git a/packages/esssans/tests/loki/workflow_test.py b/packages/esssans/tests/loki/workflow_test.py index c16b8b04a..a6141c213 100644 --- a/packages/esssans/tests/loki/workflow_test.py +++ b/packages/esssans/tests/loki/workflow_test.py @@ -161,7 +161,7 @@ def test_loki_analytical_lookup_table_matches_simulated_source_pulse(): def test_loki_workflow_reads_bool_masks_per_bank(tmp_path): path = tmp_path / 'masks.h5' mask = sc.array(dims=['tube'], values=[True, False]) - write_pixel_masks(path, {'broken': {'loki_detector_1': mask}}) + write_pixel_masks(path, {'broken': {'loki_detector_1': mask}}, instrument='LOKI') wf = loki.LokiWorkflow() wf[NeXusDetectorName] = 'loki_detector_1' wf[EmptyDetector[SampleRun]] = sc.DataArray(sc.zeros(sizes={'tube': 2})) @@ -173,7 +173,7 @@ def test_loki_workflow_reads_bool_masks_per_bank(tmp_path): def test_loki_workflow_raises_if_mask_file_has_unknown_bank(tmp_path): path = tmp_path / 'masks.h5' mask = sc.array(dims=['tube'], values=[True, False]) - write_pixel_masks(path, {'broken': {'loki_detector_99': mask}}) + write_pixel_masks(path, {'broken': {'loki_detector_99': mask}}, instrument='LOKI') wf = loki.LokiWorkflow() wf[NeXusDetectorName] = 'loki_detector_1' wf[EmptyDetector[SampleRun]] = sc.DataArray(sc.zeros(sizes={'tube': 2})) @@ -215,7 +215,7 @@ def test_bank_mask_gives_same_iofq_as_equivalent_id_file( assert 0 < masked_ids.size < ids.size bank_file = tmp_path / 'bank.nxs' - write_pixel_masks(bank_file, {'m': {'larmor_detector': mask}}) + write_pixel_masks(bank_file, {'m': {'larmor_detector': mask}}, instrument='LARMOR') id_file = tmp_path / 'ids.h5' sc.io.save_hdf5(masked_ids, id_file) @@ -232,7 +232,7 @@ def test_bool_mask_file_gives_same_masks_as_id_file(larmor_workflow, tmp_path): from_ids = next(iter(wf.compute(DetectorMasks).values())) path = tmp_path / 'masks.h5' - write_pixel_masks(path, {'m': {'larmor_detector': from_ids}}) + write_pixel_masks(path, {'m': {'larmor_detector': from_ids}}, instrument='LARMOR') wf[PixelMaskFilenames] = [path] assert sc.identical(wf.compute(DetectorMasks)['m'], from_ids) From 83d325e929ebf36929ab4050d8560faccf8c6dbd Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Thu, 12 Nov 2026 06:49:58 +0000 Subject: [PATCH 16/16] Write creator and creator_version separately in mask files NeXus defines the root attributes `creator` (program that wrote the file) and `creator_version` (its version) as separate fields. Co-Authored-By: Claude Sonnet 5.5 --- .../docs/developer/adr/0001-pixel-mask-file-format.md | 4 ++-- packages/essreduce/src/ess/reduce/masking.py | 8 +++++--- packages/essreduce/tests/masking_test.py | 4 +++- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md b/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md index d2f5d0256..a2ae02ee7 100644 --- a/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md +++ b/packages/essreduce/docs/developer/adr/0001-pixel-mask-file-format.md @@ -20,7 +20,7 @@ The planned masking tool for LoKI works on tubes and straws of a bank, after the A mask file is an HDF5 file with the group structure of NeXus: ```text -/ attributes file_time and creator +/ attributes file_time, creator and creator_version /entry NXentry valid_from "2026-11-01T00:00:00Z", not read by workflows instrument NXinstrument @@ -59,7 +59,7 @@ with h5py.File(path) as f: Each mask has its own dimensions, so a tube mask and a straw mask need not be combined into one array. The sizes are checked when masks are applied, because scipp ignores a mask with a dimension that the data lacks, without an error. - A file with several `NXentry` groups is an error, since the reader would use only one of them and leave pixels unmasked. -- As in raw data files, `/entry/instrument/name` holds the name of the instrument, and the NeXus root attributes `file_time` and `creator` hold the time and the software that wrote the file. +- As in raw data files, `/entry/instrument/name` holds the name of the instrument, and the NeXus root attributes `file_time`, `creator` and `creator_version` hold the time, the software and the software version that wrote the file. Bank names alone do not always make the instrument obvious. `file_time` gives a rough idea of whether a file may still be valid, also when it has no `valid_from`. `write_pixel_masks` writes all three, and workflows do not read them. diff --git a/packages/essreduce/src/ess/reduce/masking.py b/packages/essreduce/src/ess/reduce/masking.py index 17ef5b889..a58403f8e 100644 --- a/packages/essreduce/src/ess/reduce/masking.py +++ b/packages/essreduce/src/ess/reduce/masking.py @@ -146,8 +146,9 @@ def write_pixel_masks( unsigned 8-bit integers, since HDF5 has no boolean type. As in raw data files, the file records the instrument name in - ``/entry/instrument/name``, and the time and software that wrote it in the root - attributes ``file_time`` and ``creator``. Workflows do not read them. + ``/entry/instrument/name``, and the time, software and software version that wrote + it in the root attributes ``file_time``, ``creator`` and ``creator_version``. + Workflows do not read them. Parameters ---------- @@ -162,7 +163,8 @@ def write_pixel_masks( """ with h5py.File(filename, 'w') as h5file: h5file.attrs['file_time'] = datetime.now(UTC).isoformat(timespec='seconds') - h5file.attrs['creator'] = f'essreduce {__version__}' + h5file.attrs['creator'] = 'essreduce' + h5file.attrs['creator_version'] = __version__ entry = snx.File(h5file).create_class('entry', snx.NXentry) nx_instrument = entry.create_class('instrument', snx.NXinstrument) nx_instrument.create_field('name', instrument) diff --git a/packages/essreduce/tests/masking_test.py b/packages/essreduce/tests/masking_test.py index 4038f52d9..76590ef80 100644 --- a/packages/essreduce/tests/masking_test.py +++ b/packages/essreduce/tests/masking_test.py @@ -9,6 +9,7 @@ import scipp as sc import scippnexus as snx +from ess.reduce import __version__ from ess.reduce.masking import ( PixelMaskFilenames, PixelMasks, @@ -193,7 +194,8 @@ def test_write_records_instrument_creator_and_time(tmp_path: Path) -> None: write_pixel_masks(path, {'m': {'bank_0': mask}}, instrument='LOKI') with h5py.File(path) as file: assert file['entry/instrument/name'][()] == b'LOKI' - assert file.attrs['creator'].startswith('essreduce ') + assert file.attrs['creator'] == 'essreduce' + assert file.attrs['creator_version'] == __version__ assert before <= datetime.fromisoformat(file.attrs['file_time'])