diff --git a/src/pyhf/modifiers/histosys.py b/src/pyhf/modifiers/histosys.py index d23e6c1ff3..78b0f20074 100644 --- a/src/pyhf/modifiers/histosys.py +++ b/src/pyhf/modifiers/histosys.py @@ -1,10 +1,10 @@ import logging import pyhf -from pyhf import events -from pyhf.tensor.manager import get_backend -from pyhf import interpolators +from pyhf import events, interpolators +from pyhf.exceptions import InvalidModifier from pyhf.parameters import ParamViewer +from pyhf.tensor.manager import get_backend log = logging.getLogger(__name__) @@ -61,8 +61,8 @@ def append(self, key, channel, sample, thismod, defined_samp): def finalize(self): default_backend = pyhf.default_backend - for modifier in self.builder_data.values(): - for sample in modifier.values(): + for modifier_name, modifier in self.builder_data.items(): + for sample_name, sample in modifier.items(): sample["data"]["mask"] = default_backend.concatenate( sample["data"]["mask"] ) @@ -75,6 +75,21 @@ def finalize(self): sample["data"]["nom_data"] = default_backend.concatenate( sample["data"]["nom_data"] ) + if ( + not len(sample["data"]["nom_data"]) + == len(sample["data"]["lo_data"]) + == len(sample["data"]["hi_data"]) + ): + _modifier_type, _modifier_name = modifier_name.split("/") + _sample_data_len = len(sample["data"]["nom_data"]) + _lo_data_len = len(sample["data"]["lo_data"]) + _hi_data_len = len(sample["data"]["hi_data"]) + raise InvalidModifier( + f"The '{sample_name}' sample {_modifier_type} modifier" + + f" '{_modifier_name}' has data shape inconsistent with the sample.\n" + + f"{sample_name} has 'data' of length {_sample_data_len} but {_modifier_name}" + + f" has 'lo_data' of length {_lo_data_len} and 'hi_data' of length {_hi_data_len}." + ) return self.builder_data diff --git a/src/pyhf/modifiers/shapesys.py b/src/pyhf/modifiers/shapesys.py index 5b5466857f..79e2c40e67 100644 --- a/src/pyhf/modifiers/shapesys.py +++ b/src/pyhf/modifiers/shapesys.py @@ -2,8 +2,9 @@ import pyhf from pyhf import events -from pyhf.tensor.manager import get_backend +from pyhf.exceptions import InvalidModifier from pyhf.parameters import ParamViewer +from pyhf.tensor.manager import get_backend log = logging.getLogger(__name__) @@ -70,8 +71,8 @@ def append(self, key, channel, sample, thismod, defined_samp): def finalize(self): default_backend = pyhf.default_backend - for modifier in self.builder_data.values(): - for sample in modifier.values(): + for modifier_name, modifier in self.builder_data.items(): + for sample_name, sample in modifier.items(): sample["data"]["mask"] = default_backend.concatenate( sample["data"]["mask"] ) @@ -81,6 +82,16 @@ def finalize(self): sample["data"]["nom_data"] = default_backend.concatenate( sample["data"]["nom_data"] ) + if len(sample["data"]["nom_data"]) != len(sample["data"]["uncrt"]): + _modifier_type, _modifier_name = modifier_name.split("/") + _sample_data_len = len(sample["data"]["nom_data"]) + _uncrt_len = len(sample["data"]["uncrt"]) + raise InvalidModifier( + f"The '{sample_name}' sample {_modifier_type} modifier" + + f" '{_modifier_name}' has data shape inconsistent with the sample.\n" + + f"{sample_name} has 'data' of length {_sample_data_len} but {_modifier_name}" + + f" has 'data' of length {_uncrt_len}." + ) return self.builder_data diff --git a/src/pyhf/modifiers/staterror.py b/src/pyhf/modifiers/staterror.py index c6a7bfb9ba..3cb1cd5c4e 100644 --- a/src/pyhf/modifiers/staterror.py +++ b/src/pyhf/modifiers/staterror.py @@ -3,8 +3,9 @@ import pyhf from pyhf import events -from pyhf.tensor.manager import get_backend +from pyhf.exceptions import InvalidModifier from pyhf.parameters import ParamViewer +from pyhf.tensor.manager import get_backend log = logging.getLogger(__name__) @@ -54,8 +55,8 @@ def append(self, key, channel, sample, thismod, defined_samp): def finalize(self): default_backend = pyhf.default_backend - for modifier in self.builder_data.values(): - for sample in modifier.values(): + for modifier_name, modifier in self.builder_data.items(): + for sample_name, sample in modifier.items(): sample["data"]["mask"] = default_backend.concatenate( sample["data"]["mask"] ) @@ -65,6 +66,16 @@ def finalize(self): sample["data"]["nom_data"] = default_backend.concatenate( sample["data"]["nom_data"] ) + if len(sample["data"]["nom_data"]) != len(sample["data"]["uncrt"]): + _modifier_type, _modifier_name = modifier_name.split("/") + _sample_data_len = len(sample["data"]["nom_data"]) + _uncrt_len = len(sample["data"]["uncrt"]) + raise InvalidModifier( + f"The '{sample_name}' sample {_modifier_type} modifier" + + f" '{_modifier_name}' has data shape inconsistent with the sample.\n" + + f"{sample_name} has 'data' of length {_sample_data_len} but {_modifier_name}" + + f" has 'data' of length {_uncrt_len}." + ) for modname in self.builder_data.keys(): parname = modname.split('/')[1] diff --git a/tests/test_modifiers.py b/tests/test_modifiers.py index 1e0c7b81a7..9cf4fffe07 100644 --- a/tests/test_modifiers.py +++ b/tests/test_modifiers.py @@ -1,15 +1,10 @@ -import pyhf +import json + import numpy +import pytest +from jsonpatch import JsonPatch -modifiers_to_test = [ - "histosys", - "normfactor", - "normsys", - "shapefactor", - "shapesys", - "staterror", -] -modifier_pdf_types = ["normal", None, "normal", None, "poisson", "normal"] +import pyhf def test_shapefactor_build(): @@ -174,3 +169,27 @@ def test_shapesys_holes(): True, False, ] + + +@pytest.mark.parametrize( + "patch_file", + [ + "bad_histosys_modifier_patch.json", + "bad_shapesys_modifier_patch.json", + "bad_staterror_modifier_patch.json", + ], +) +def test_invalid_bin_wise_modifier(datadir, patch_file): + """ + Test that bin-wise modifiers will raise an exception if their data shape + differs from their sample's. + """ + spec = json.load(open(datadir.join("spec.json"))) + + assert pyhf.Model(spec) + + patch = JsonPatch.from_string(open(datadir.join(patch_file)).read()) + bad_spec = patch.apply(spec) + + with pytest.raises(pyhf.exceptions.InvalidModifier): + pyhf.Model(bad_spec) diff --git a/tests/test_modifiers/bad_histosys_modifier_patch.json b/tests/test_modifiers/bad_histosys_modifier_patch.json new file mode 100644 index 0000000000..7b1ca4a4a0 --- /dev/null +++ b/tests/test_modifiers/bad_histosys_modifier_patch.json @@ -0,0 +1,24 @@ +[ + { + "op": "add", + "path": "/channels/0/samples/1/modifiers", + "value": [ + { + "name": "histosys_bad", + "type": "histosys", + "data": { + "hi_data": [ + 3, + 6, + 9 + ], + "lo_data": [ + 1, + 2, + 3 + ] + } + } + ] + } +] diff --git a/tests/test_modifiers/bad_shapesys_modifier_patch.json b/tests/test_modifiers/bad_shapesys_modifier_patch.json new file mode 100644 index 0000000000..c73bb0331c --- /dev/null +++ b/tests/test_modifiers/bad_shapesys_modifier_patch.json @@ -0,0 +1,17 @@ +[ + { + "op": "add", + "path": "/channels/0/samples/1/modifiers", + "value": [ + { + "name": "shapesys_bad", + "type": "shapesys", + "data": [ + 1, + 2, + 3 + ] + } + ] + } +] diff --git a/tests/test_modifiers/bad_staterror_modifier_patch.json b/tests/test_modifiers/bad_staterror_modifier_patch.json new file mode 100644 index 0000000000..c10b816d4b --- /dev/null +++ b/tests/test_modifiers/bad_staterror_modifier_patch.json @@ -0,0 +1,17 @@ +[ + { + "op": "add", + "path": "/channels/0/samples/1/modifiers", + "value": [ + { + "name": "staterror_bad", + "type": "staterror", + "data": [ + 1, + 2, + 3 + ] + } + ] + } +] diff --git a/tests/test_modifiers/spec.json b/tests/test_modifiers/spec.json new file mode 100644 index 0000000000..ede90899e1 --- /dev/null +++ b/tests/test_modifiers/spec.json @@ -0,0 +1,35 @@ +{ + "channels": [ + { + "name": "channel_1", + "samples": [ + { + "name": "sample_1", + "data": [ + 1, + 2, + 3, + 4 + ], + "modifiers": [ + { + "name": "mu", + "type": "normfactor", + "data": null + } + ] + }, + { + "name": "sample_2", + "data": [ + 2, + 4, + 6, + 8 + ], + "modifiers": [] + } + ] + } + ] +}