-
Notifications
You must be signed in to change notification settings - Fork 83
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
feat: Don't specify a default POI name #2328
Conversation
src/pyhf/simplemodels.py
Outdated
@@ -79,7 +79,7 @@ def correlated_background( | |||
} | |||
] | |||
} | |||
return Model(spec, batch_size=batch_size, validate=validate) | |||
return Model(spec, batch_size=batch_size, validate=validate, poi_name="mu") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to be changing the default behaivor of pyhf.pdf.Model
should we just go all in and make sure that pyhf.simplemodel
also requires users to set things? Or should we keep the pyhf.simplemodel
experience the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fair to automatically set the POI for the simplemodels
case. There is no ambiguity there for what else the POI should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could elevate this to a user accesible argument that defaults to mu
and pass that through here, but as you said, the POI for these models is meant to be mu
.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2328 +/- ##
==========================================
+ Coverage 98.14% 98.28% +0.13%
==========================================
Files 69 69
Lines 4538 4538
Branches 803 803
==========================================
+ Hits 4454 4460 +6
+ Misses 48 45 -3
+ Partials 36 33 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
e59695e
to
b863194
Compare
b863194
to
951ad7a
Compare
* Ensure all pyhf.Model calls in the example notebooks that don't already have a poi_name definied have `poi_name="mu"` set. * Amends PR #2328
Description
Resolves #2327
To avoid making assumptions for users about model behavior set the default POI name to
None
.To avoid making behavior changes for
pyhf.simplemodels
add the defaultpoi_name
to the returned model.This then requires explicilty setting
poi_name="mu"
in the tests for all explicitypyhf.pdf.Model
constrcution.Checklist Before Requesting Reviewer
Before Merging
For the PR Assignees: