refactor: Simplified parameters #1639
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
matthewfeickert
added
API
matthewfeickert
force-pushed
the
simplied_parameters
branch
from
bee1f09 to
a51ad0e
Compare
matthewfeickert
changed the title
|
@lukasheinrich This has now been rebased off of |
lukasheinrich
force-pushed
the
simplied_parameters
branch
2 times, most recently
from
dae1fab to
00d70d3
Compare
Codecov Report
@@ Coverage Diff @@
## master #1639 +/- ##
=======================================
Coverage 98.07% 98.07%
=======================================
Files 64 64
Lines 4199 4216 +17
Branches 578 585 +7
=======================================
+ Hits 4118 4135 +17
Misses 48 48
Partials 33 33
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
@alexander-held this resolves #856 and adds tests for this: https://github.com/scikit-hep/pyhf/pull/1639/files#diff-e45665cfa3703a05ae8cc625bd9f638d7694398a2aa92eab9b2bfaaf34a99e6fR43 |
|
|
||
|
|
||
| def test_staterror_holes(): | ||
| spec = { |
There was a problem hiding this comment.
When fitting this to Asimov:
pyhf.set_backend(pyhf.tensorlib, pyhf.optimize.minuit_optimizer(verbose=2))
res = pyhf.infer.mle.fit(
[50.0, 60.0, 0.0, 70.0, 50.0, 60.0, 10.0, 70.0] + model.config.auxdata,
model,
return_uncertainties=True,
)
for i, parname in enumerate(model.config.par_names()):
print(f"{parname} = {res[i][0]} +/- {res[i][1]}")it looks like things do not work as expected yet. When making all staterrors non-zero, the fit will still fail if the prediction for the third bin of channel1 is 0 (this is probably independent of this PR).
When making the model prediction non-zero everywhere, but making one staterror bin zero again, the fit will fail as the pdf is NaN, but the parameter for the bin with zero staterror is still a free parameter in the fit. I would have expected it to be held constant or pruned instead.
There was a problem hiding this comment.
that's a good catch - I need to add something like reindex_access_field to staterror.. I have removed the test for now.
So I guess the logic we want is that the NP is removed if the sum of the nominals across all sampples is zero
There was a problem hiding this comment.
For staterror it could be removed in two cases:
- nominal is zero (then the modifier has no effect anyway, though the parameter may act on other samples still)
- auxdata is zero (again, the parameter may act on other samples still)
If the parameter does not modify any samples at all, then I think it should be pruned.
I was also looking at the naming: it may be nice to change the parameter component names such that the index corresponds to the bin. For three parameters ["par[0]", "par[1]", "par[2]"] acting on three bins, I would expect the parameter names to be ["par[0]", "par[2]"] when removing the parameter acting on the central bin, instead of the ["par[0]", "par[1]"] that seems to be used now (which may make it difficult to track which parameters still exist).
There was a problem hiding this comment.
import pyhf
import matplotlib.pyplot as plt
spec = {
'channels': [
{
'name': 'channel1',
'samples': [
{
'name': 'another_sample',
'data': [50,60,0,70],
'modifiers': [
{'name': 'mu', 'type': 'normfactor', 'data': None},
{'name': 'staterror_1', 'type': 'staterror', 'data': [5,0,5,5]}
],
},
],
},
{
'name': 'channel2',
'samples': [
{
'name': 'another_sample',
'data': [50,0,0,70],
'modifiers': [
{'name': 'staterror_2', 'type': 'staterror', 'data': [5,0,5,5]}
],
},
],
}
],
}
model = pyhf.Model(spec, poi_name = '')
assert model.config.npars == 6
a,b = model._modifications(pyhf.tensorlib.astensor([10,2.,3.,4.,5.,6.]))
assert (b[1][0,0,0,:] == [2.,3.,1.,4.,1.,1.,1.,1.]).all()
assert (b[1][1,0,0,:] == [1.,1.,1.,1.,5.,1.,1.,6.]).all()
plt.imshow(b[1][:,0,0,:])
gives:
There was a problem hiding this comment.
ok re-added the test @alexander-held
There was a problem hiding this comment.
disucssion here: #1650
lukasheinrich
force-pushed
the
simplied_parameters
branch
4 times, most recently
from
f3647d2 to
a98d946
Compare
kratsg
force-pushed
the
simplied_parameters
branch
from
d03c741 to
1e07e2b
Compare
for more information, see https://pre-commit.ci
matthewfeickert
added
tests
There was a problem hiding this comment.
LGTM. 👍 Thanks @lukasheinrich!
Once the CI finishes I'll merge this in. 🚀
matthewfeickert
changed the title
|
For future reference, this changed the behavior of |
… of 0 (#1740) * Update warning note for shapesys modifier, and add similar warning for staterror modifier, that bins that have a modifier data value of 0 will result in a nuisance parameter being allocated resulting in a fixed modifier of 1. - These changes occurred in PR #1639 * Move descriptions of modifier examples before example is given for clarity.
* Fix staterror bug introduced after v0.6.3 where staterror erroneously affected multiple samples. - Amends PR #1639 which introduced regression. - c.f. Issue #1720 * Remove staterror_combined.__staterror_uncrt as it is unused by the codebase. * Add test to verify that staterror does not affect more samples than shapesys equivalently. Co-authored-by: Giordon Stark <kratsg@gmail.com>
Description
this is just a commit on top of #1625 that cleans up parameter handling and will a lot of nice things (user-configurable constraint terms, dropping constraints, etc)
Checklist Before Requesting Reviewer
Before Merging
For the PR Assignees: