Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can no longer set several model config attributes #1984

Closed
1 task done
alexander-held opened this issue Sep 5, 2022 · 4 comments · Fixed by #1985
Closed
1 task done

Can no longer set several model config attributes #1984

alexander-held opened this issue Sep 5, 2022 · 4 comments · Fixed by #1985
Assignees
Labels
API Changes the public API bug Something isn't working

Comments

@alexander-held
Copy link
Member

alexander-held commented Sep 5, 2022

Summary

The changes in #1972 cause model.config.poi_index to not be writable anymore. cabinetry sets this value directly to support running pyhf.infer.hypotest with a configurable POI (without the need to re-build the model).

OS / Environment

n/a

Steps to Reproduce

import pyhf

model = pyhf.simplemodels.uncorrelated_background(
    signal=[24.0, 22.0], bkg=[50.0, 52.0], bkg_uncertainty=[3.0, 7.0]
)
model.config.poi_index = 1

File Upload (optional)

No response

Expected Results

assignment succeeds

Actual Results

Traceback (most recent call last):
  File "test.py", line 7, in <module>
    model.config.poi_index = ""
AttributeError: can't set attribute

pyhf Version

pyhf, version 0.7.0rc4.dev2

Code of Conduct

  • I agree to follow the Code of Conduct
@alexander-held alexander-held added bug Something isn't working needs-triage Needs a maintainer to categorize and assign labels Sep 5, 2022
@alexander-held
Copy link
Member Author

This affects also model.config.poi_name and presumably the rest of the attributes that were decorated with @property: par_order, auxdata, nmaindata, nauxdata, as well as model.config itself. I could imagine people possibly wanting to override auxdata and potentially the full model.config, I have not had the need to do so myself.

Adding the following two setters restores the functionality cabinetry uses at the moment:

diff --git a/src/pyhf/pdf.py b/src/pyhf/pdf.py
index db59fa1c..969b26db 100644
--- a/src/pyhf/pdf.py
+++ b/src/pyhf/pdf.py
@@ -245,6 +245,13 @@ def poi_name(self):
         """
         return self._poi_name

+    @poi_name.setter
+    def poi_name(self, value):
+        """
+        Set the name of the POI parameter in the model.
+        """
+        self._poi_name = value
+
     @property
     def poi_index(self):
         """
@@ -252,6 +259,13 @@ def poi_index(self):
         """
         return self._poi_index

+    @poi_index.setter
+    def poi_index(self, value):
+        """
+        Set the index of the POI parameter in the model.
+        """
+        self._poi_index = value
+
     @property
     def auxdata(self):
         """

@alexander-held alexander-held changed the title Can no longer set model.config.poi_index Can no longer set several model config attributes Sep 5, 2022
@matthewfeickert matthewfeickert added API Changes the public API and removed needs-triage Needs a maintainer to categorize and assign labels Sep 5, 2022
@matthewfeickert
Copy link
Member

When this is fixed there should additionally be a test that is added to ensure that these can be set by the user.

@kratsg
Copy link
Contributor

kratsg commented Sep 5, 2022

This is not a bug. The API for setting the poi_index is done through model.config.set_poi(name: str). https://scikit-hep.org/pyhf/_generated/pyhf.pdf._ModelConfig.html#pyhf.pdf._ModelConfig.set_poi

We never encouraged anyone to override these other properties you mentioned either. The whole point is to try and make Model less dynamically configurable because there was always confusion about how mutable it was meant to be.

@alexander-held
Copy link
Member Author

alexander-held commented Sep 5, 2022

Making this explicit by using set_poi is a good solution, but it currently does not support setting POIs back to "no POI" mode where poi_name / poi_index are None. This worked when setting the attribute directly so far.

import pyhf

spec = {
    "channels": [
        {
            "name": "SR",
            "samples": [
                {
                    "data": [50],
                    "modifiers": [
                        {"data": None, "name": "mu", "type": "normfactor"},
                    ],
                    "name": "Signal",
                },
            ],
        }
    ],
    "measurements": [
        {
            "config": {
                "parameters": [],
                "poi": "",
            },
            "name": "meas",
        }
    ],
    "observations": [{"data": [50], "name": "SR"}],
    "version": "1.0.0",
}

ws = pyhf.Workspace(spec)
model = ws.model()
data = ws.data(model)

print(model.config.poi_name is None)  # True
print(model.config.poi_index is None)  # True

model.config.set_poi("mu")  # temporarily set for e.g. hypotest
pyhf.infer.hypotest(1.0, data, model)

# neither of the following works, expects string for a parameter that is in the model
# model.config.set_poi(None)
# model.config.set_poi("")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Changes the public API bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants