feat: Support ShapeFactor for import/export (#1136)

* Add ShapeFactor support to pyhf.readxml
* Add ShapeFactor support to pyhf.writexml
* Add ShapeFactor test

src/pyhf/readxml.py

@@ -171,6 +171,10 @@ def process_sample(
                     'data': [a * b for a, b in zip(data, shapesys_data)],
                 }
             )
+        elif modtag.tag == 'ShapeFactor':
+            modifiers.append(
+                {'name': modtag.attrib['Name'], 'type': 'shapefactor', 'data': None}
+            )
         else:
             log.warning('not considering modifier tag %s', modtag)
 

src/pyhf/writexml.py

@@ -205,12 +205,15 @@ def build_modifier(spec, modifierspec, channelname, samplename, sampledata):
                 for a, b in np.array((modifierspec['data'], sampledata)).T
             ],
         )
+    elif modifierspec['type'] == 'shapefactor':
+        pass
     else:
         log.warning(
-            'Skipping {0}({1}) for now'.format(
+            'Skipping modifier {0}({1}) for now'.format(
                 modifierspec['name'], modifierspec['type']
             )
         )
+        return None
 
     modifier = ET.Element(mod_map[modifierspec['type']], **attrs)
     return modifier

tests/test_export.py

@@ -3,6 +3,7 @@
 import pytest
 import json
 import xml.etree.cElementTree as ET
+import logging
 
 
 def spec_staterror():
@@ -143,6 +144,34 @@ def spec_shapesys():
     return spec
 
 
+def spec_shapefactor():
+    source = json.load(open('validation/data/2bin_histosys_example2.json'))
+    spec = {
+        'channels': [
+            {
+                'name': 'singlechannel',
+                'samples': [
+                    {
+                        'name': 'signal',
+                        'data': source['bindata']['sig'],
+                        'modifiers': [
+                            {'name': 'mu', 'type': 'normfactor', 'data': None}
+                        ],
+                    },
+                    {
+                        'name': 'background',
+                        'data': source['bindata']['bkg'],
+                        'modifiers': [
+                            {'name': 'bkg_norm', 'type': 'shapefactor', 'data': None}
+                        ],
+                    },
+                ],
+            }
+        ]
+    }
+    return spec
+
+
 def test_export_measurement():
     measurementspec = {
         "config": {
@@ -198,10 +227,11 @@ def test_export_measurement():
         (spec_histosys(), True, ['HistoNameHigh', 'HistoNameLow']),
         (spec_normsys(), False, ['High', 'Low']),
         (spec_shapesys(), True, ['ConstraintType', 'HistoName']),
+        (spec_shapefactor(), False, []),
     ],
-    ids=['staterror', 'histosys', 'normsys', 'shapesys'],
+    ids=['staterror', 'histosys', 'normsys', 'shapesys', 'shapefactor'],
 )
-def test_export_modifier(mocker, spec, has_root_data, attrs):
+def test_export_modifier(mocker, caplog, spec, has_root_data, attrs):
     channelspec = spec['channels'][0]
     channelname = channelspec['name']
     samplespec = channelspec['samples'][1]
@@ -210,20 +240,36 @@ def test_export_modifier(mocker, spec, has_root_data, attrs):
     modifierspec = samplespec['modifiers'][0]
 
     mocker.patch('pyhf.writexml._ROOT_DATA_FILE')
-    modifier = pyhf.writexml.build_modifier(
-        {'measurements': [{'config': {'parameters': []}}]},
-        modifierspec,
-        channelname,
-        samplename,
-        sampledata,
-    )
+
+    with caplog.at_level(logging.DEBUG, 'pyhf.writexml'):
+        modifier = pyhf.writexml.build_modifier(
+            {'measurements': [{'config': {'parameters': []}}]},
+            modifierspec,
+            channelname,
+            samplename,
+            sampledata,
+        )
+    assert "Skipping modifier" not in caplog.text
+
     # if the modifier is a staterror, it has no Name
     if 'Name' in modifier.attrib:
         assert modifier.attrib['Name'] == modifierspec['name']
     assert all(attr in modifier.attrib for attr in attrs)
     assert pyhf.writexml._ROOT_DATA_FILE.__setitem__.called == has_root_data
 
 
+def test_export_bad_modifier(caplog):
+    with caplog.at_level(logging.DEBUG, 'pyhf.writexml'):
+        pyhf.writexml.build_modifier(
+            {'measurements': [{'config': {'parameters': []}}]},
+            {'name': 'fakeModifier', 'type': 'unknown-modifier'},
+            'fakeChannel',
+            'fakeSample',
+            None,
+        )
+    assert "Skipping modifier fakeModifier(unknown-modifier)" in caplog.text
+
+
 @pytest.mark.parametrize(
     "spec, normfactor_config",
     [

tests/test_import.py

@@ -5,6 +5,7 @@
 from pathlib import Path
 import pytest
 import xml.etree.cElementTree as ET
+import logging
 
 
 def assert_equal_dictionary(d1, d2):
@@ -322,3 +323,87 @@ def test_import_normfactor_bounds():
     assert len(parameters) == 1
     parameter = parameters[0]
     assert parameter['bounds'] == [[0, 10]]
+
+
+def test_import_shapefactor():
+    parsed_xml = pyhf.readxml.parse(
+        'validation/xmlimport_input/config/examples/example_DataDriven.xml',
+        'validation/xmlimport_input',
+    )
+
+    # build the spec, strictly checks properties included
+    spec = {
+        'channels': parsed_xml['channels'],
+        'parameters': parsed_xml['measurements'][0]['config']['parameters'],
+    }
+    pdf = pyhf.Model(spec, poi_name='SigXsecOverSM')
+
+    channels = {channel['name']: channel for channel in pdf.spec['channels']}
+
+    assert channels['controlRegion']['samples'][0]['modifiers'][0]['type'] == 'lumi'
+    assert (
+        channels['controlRegion']['samples'][0]['modifiers'][1]['type'] == 'staterror'
+    )
+    assert channels['controlRegion']['samples'][0]['modifiers'][2]['type'] == 'normsys'
+    assert (
+        channels['controlRegion']['samples'][1]['modifiers'][0]['type'] == 'shapefactor'
+    )
+
+
+def test_process_modifiers(mocker, caplog):
+    sample = ET.Element(
+        "Sample", Name='testSample', HistoPath="", HistoName="testSample"
+    )
+    normfactor = ET.Element(
+        'NormFactor', Name="myNormFactor", Val='1', Low="0", High="3"
+    )
+    histosys = ET.Element(
+        'HistoSys', Name='myHistoSys', HistoNameHigh='', HistoNameLow=''
+    )
+    normsys = ET.Element('OverallSys', Name='myNormSys', High='1.05', Low='0.95')
+    shapesys = ET.Element('ShapeSys', Name='myShapeSys', HistoName='')
+    shapefactor = ET.Element(
+        "ShapeFactor",
+        Name='myShapeFactor',
+    )
+    staterror = ET.Element('StatError', Activate='True')
+    unknown_modifier = ET.Element('UnknownSys')
+
+    sample.append(normfactor)
+    sample.append(histosys)
+    sample.append(normsys)
+    sample.append(shapesys)
+    sample.append(shapefactor)
+    sample.append(staterror)
+    sample.append(unknown_modifier)
+
+    _data = [0.0]
+    _err = [1.0]
+    mocker.patch('pyhf.readxml.import_root_histogram', return_value=(_data, _err))
+    with caplog.at_level(logging.DEBUG, 'pyhf.readxml'):
+        result = pyhf.readxml.process_sample(sample, '', '', '', 'myChannel')
+
+    assert "not considering modifier tag <Element 'UnknownSys'" in caplog.text
+    assert len(result['modifiers']) == 6
+    assert {'name': 'myNormFactor', 'type': 'normfactor', 'data': None} in result[
+        'modifiers'
+    ]
+    assert {
+        'name': 'myHistoSys',
+        'type': 'histosys',
+        'data': {'lo_data': _data, 'hi_data': _data},
+    } in result['modifiers']
+    assert {
+        'name': 'myNormSys',
+        'type': 'normsys',
+        'data': {'lo': 0.95, 'hi': 1.05},
+    } in result['modifiers']
+    assert {'name': 'myShapeSys', 'type': 'shapesys', 'data': _data} in result[
+        'modifiers'
+    ]
+    assert {'name': 'myShapeFactor', 'type': 'shapefactor', 'data': None} in result[
+        'modifiers'
+    ]
+    assert {'name': 'staterror_myChannel', 'type': 'staterror', 'data': _err} in result[
+        'modifiers'
+    ]