feat: Bookkeep fixed parameters #989

kratsg · 2020-07-26T03:40:20Z

Description

This will help with #846 .

The goal of this PR is to:

allow for pdf.config.suggested_fixed to provide a list of True/False of parameters which are fixed. This can be essentially stitched with the inits provided by the user, or pdf.config.suggested_init() to get fixed_vals for the optimizer API.
- tests are added
~~[ ] update the schema (we were idiots) to change fixed to be an array as it needs to be defined for each parameter~~
- ~~[ ] tests are added~~
- ~~we can additionally allow for a single bool and an array of bools. The single bool could provide backwards compatibility implying all parameters for the modifier are specified as fixed.~~
~~[ ] update src/pyhf/readxml.py to correctly read in ParamSetting fixed parameters correctly~~
- ~~[ ] tests are added~~
~~[ ] update src/pyhf/writexml.py to write out fixed parameters correctly against the new schema~~
- ~~[ ] tests are added~~

This PR will not propagate fixed parameters from the pdf through to the optimizers. That is the goal of #846.

Help requested:

~~we need examples of HistFactory XMLs that fixed one or two parameters of a given modifier (rather than all of them).~~
- decided for now paramsetting will fix all parameters of a given systematic

Bugs identified(?)

~~in src/pyhf/readxml.py, inits suffers a similar bug that fixed does, where the inits for a modifier with n-parameters will be a list of one value.~~
- inits (like fixed) will be for all parameters of a systematic

Brain dump:

for readxml.py: use re or parse to parse out parameter indices in ParamSetting.text values. I will need an example (see "Help requested") but I assume gamma_stat_SR0L_IStR_cuts[2] would be written into ParamSetting and we need to extract the index of the parameter that such a thing is being configured for (parameter setting) -- and if there is no indexing involved -- this is assumed for all parameters in that modifier (modifier setting).
for readxml.py: if we parse out parameter setting, we're going to need to figure out how many parameters the associated modifier has in total (only for: staterror, shapesys, shapefactor) so we can stitch this in appropriately
perhaps to preserve backwards compatibility, we can move the complexity out of readxml/writexml and handle parameter parsing inside pdf.py. Instead of trying to go from gamma_stat_SR0L_IStR_cuts[2] to gamma_stat_SR0L_IStR_cuts (or backwards) in read/write XML... we can just write out gamma_stat_SR0L_IStR_cuts[2] as literally the name of the parameter configuration which our JSON is technically for, rather than modifier configuration. As such, every parameter is single-bin -- so the complexity is moved over to trying to reinflate these parameters and inject them into the corresponding modifiers.

ReadTheDocs build: https://pyhf.readthedocs.io/en/feat-bookkeepfixedparameters

Checklist Before Requesting Reviewer

Tests are passing
"WIP" removed from the title of the pull request
Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

Summarize commit messages into a comprehensive review of the PR

* Add functionality into _ModelConfig to bookkeep fixed systematics (all parameters for a given systematic)
* Add fixed into tests for paramsets
* Add tests for fixed parameters

codecov · 2020-07-26T04:53:57Z

Codecov Report

Merging #989 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #989   +/-   ##
=======================================
  Coverage   96.65%   96.66%           
=======================================
  Files          59       59           
  Lines        3287     3294    +7     
  Branches      455      456    +1     
=======================================
+ Hits         3177     3184    +7     
  Misses         69       69           
  Partials       41       41

Flag	Coverage Δ
#unittests	`96.66% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/pyhf/modifiers/histosys.py	`100.00% <ø> (ø)`
src/pyhf/modifiers/lumi.py	`100.00% <ø> (ø)`
src/pyhf/modifiers/normfactor.py	`100.00% <ø> (ø)`
src/pyhf/modifiers/normsys.py	`100.00% <ø> (ø)`
src/pyhf/modifiers/shapefactor.py	`100.00% <ø> (ø)`
src/pyhf/modifiers/shapesys.py	`100.00% <ø> (ø)`
src/pyhf/modifiers/staterror.py	`96.61% <ø> (ø)`
src/pyhf/parameters/utils.py	`92.59% <ø> (ø)`
src/pyhf/parameters/paramsets.py	`100.00% <100.00%> (ø)`
src/pyhf/pdf.py	`95.93% <100.00%> (+0.08%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fcf95b...ada372a. Read the comment docs.

src/pyhf/modifiers/staterror.py

src/pyhf/modifiers/shapesys.py

src/pyhf/modifiers/shapefactor.py

src/pyhf/parameters/paramsets.py

matthewfeickert

Looks good. As you said @kratsg, this actually isn't too big, just needing to put these decisions in place. Mostly just a docs fix and then some questions from me but basically good to go.

src/pyhf/pdf.py

tests/test_schema.py

tests/test_pdf.py

src/pyhf/modifiers/shapefactor.py

This reverts commit 90f465e.

Co-authored-by: Matthew Feickert <matthew.feickert@cern.ch>

matthewfeickert

LGTM

src/pyhf/parameters/paramsets.py

src/pyhf/parameters/utils.py

src/pyhf/pdf.py

lukasheinrich · 2020-08-02T08:59:26Z

LGTM

alexander-held · 2020-08-03T12:25:56Z

Hi, just gave this a try. I think pdf.config.fixed_pars in the description should mean pdf.config.suggested_fixed(). I wonder whether "suggested" could be misleading, but I guess it depends on whether one views the measurements part of the workspace as a suggestion for how to perform a measurement, or one views the whole workspace as an instruction for doing one specific fit (I personally perceive it more like the second option).

matthewfeickert · 2020-08-03T20:50:00Z

Hi, just gave this a try. I think pdf.config.fixed_pars in the description should mean pdf.config.suggested_fixed(). I wonder whether "suggested" could be misleading, but I guess it depends on whether one views the measurements part of the workspace as a suggestion for how to perform a measurement, or one views the whole workspace as an instruction for doing one specific fit (I personally perceive it more like the second option).

@alexander-held Can you open this up as an Issue for us to discuss there (also hopefully making it easier for others to find for future reference)?

kratsg · 2020-08-03T20:54:12Z

Hi, just gave this a try. I think pdf.config.fixed_pars in the description should mean pdf.config.suggested_fixed()

Fixed.

I wonder whether "suggested" could be misleading, but I guess it depends on whether one views the measurements part of the workspace as a suggestion for how to perform a measurement, or one views the whole workspace as an instruction for doing one specific fit (I personally perceive it more like the second option).

Part of this is to separate out a model definition from a measurement definition. The measurement definitions come into play with suggested_xyz accessors -- as you probably saw (inits, bounds, fixed).

alexander-held · 2020-08-03T20:56:07Z

I'll open an issue as suggested by @matthewfeickert, but in the end it's matter of taste what to call it I think.

kratsg added the feat/enhancement New feature or request label Jul 26, 2020

kratsg mentioned this pull request Jul 26, 2020

Add support for multiple parameters of interest #179

Open

matthewfeickert changed the title ~~WIP: feat: Bookkeep fixed parameters~~ feat: Bookkeep fixed parameters Jul 27, 2020

matthewfeickert assigned kratsg Jul 27, 2020

lukasheinrich force-pushed the master branch from 27f35e9 to e55eea4 Compare July 27, 2020 12:16

kratsg force-pushed the feat/bookkeepFixedParameters branch 2 times, most recently from d2958e0 to aa0e894 Compare July 30, 2020 01:49

kratsg marked this pull request as ready for review July 30, 2020 02:13

kratsg requested review from lukasheinrich and matthewfeickert July 30, 2020 02:13

kratsg commented Jul 30, 2020

View reviewed changes

src/pyhf/modifiers/staterror.py Show resolved Hide resolved

kratsg commented Jul 30, 2020

View reviewed changes

src/pyhf/modifiers/shapesys.py Show resolved Hide resolved

kratsg commented Jul 30, 2020

View reviewed changes

src/pyhf/modifiers/shapefactor.py Show resolved Hide resolved

kratsg commented Jul 30, 2020

View reviewed changes

src/pyhf/parameters/paramsets.py Show resolved Hide resolved

matthewfeickert added the tests pytest label Jul 30, 2020

matthewfeickert requested changes Jul 30, 2020

View reviewed changes

src/pyhf/pdf.py Outdated Show resolved Hide resolved

tests/test_schema.py Show resolved Hide resolved

tests/test_pdf.py Outdated Show resolved Hide resolved

src/pyhf/modifiers/shapefactor.py Show resolved Hide resolved

kratsg added 11 commits July 30, 2020 08:15

update pdf to correctly bookkeep fixed parameters

0271fe6

update tests to check for fixed parameters

410fa11

update our schema sadly

7616a5c

single list item for now

4f7dfd3

missing slots

e8eb3c8

be more specific in this test

2765a88

fix test

3311abe

single list item for now for overall

79929cf

black

2d6d30d

fix up import test

b272f0f

get patchset tests working

12f39aa

kratsg and others added 9 commits July 30, 2020 08:15

clean up fixed_pars

6ea7306

codefactor

5fea7a5

Revert "get the scripts working"

7e126d6

This reverts commit 90f465e.

fix schema test

f45b455

need to scale out by number of pars

9c56529

add test to check

3bc4519

add docstring

c027822

Apply suggestions from code review

44bb785

Co-authored-by: Matthew Feickert <matthew.feickert@cern.ch>

update the test with more physical inits

d9f0e4c

kratsg force-pushed the feat/bookkeepFixedParameters branch from 157a28d to d9f0e4c Compare July 30, 2020 12:16

fixed_pars->suggested_fixed

34bb9a3

matthewfeickert approved these changes Jul 30, 2020

View reviewed changes

lukasheinrich reviewed Jul 31, 2020

View reviewed changes

src/pyhf/parameters/paramsets.py Outdated Show resolved Hide resolved

lukasheinrich reviewed Jul 31, 2020

View reviewed changes

src/pyhf/parameters/paramsets.py Outdated Show resolved Hide resolved

remove slots

b6e338e

lukasheinrich reviewed Aug 1, 2020

View reviewed changes

src/pyhf/parameters/utils.py Show resolved Hide resolved

lukasheinrich reviewed Aug 1, 2020

View reviewed changes

src/pyhf/pdf.py Outdated Show resolved Hide resolved

remove more slots

ada372a

lukasheinrich approved these changes Aug 2, 2020

View reviewed changes

lukasheinrich merged commit 4ef9bfc into master Aug 2, 2020

lukasheinrich deleted the feat/bookkeepFixedParameters branch August 2, 2020 08:59

alexander-held mentioned this pull request Aug 3, 2020

feat: support fixed parameters scikit-hep/cabinetry#82

Merged

alexander-held mentioned this pull request Aug 3, 2020

"Suggested" settings in measurement #1017

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Bookkeep fixed parameters #989

feat: Bookkeep fixed parameters #989

kratsg commented Jul 26, 2020 •

edited

Loading

codecov bot commented Jul 26, 2020 •

edited

Loading

matthewfeickert left a comment

matthewfeickert left a comment

lukasheinrich commented Aug 2, 2020

alexander-held commented Aug 3, 2020

matthewfeickert commented Aug 3, 2020

kratsg commented Aug 3, 2020

alexander-held commented Aug 3, 2020

feat: Bookkeep fixed parameters #989

feat: Bookkeep fixed parameters #989

Conversation

kratsg commented Jul 26, 2020 • edited Loading

Description

Checklist Before Requesting Reviewer

Before Merging

codecov bot commented Jul 26, 2020 • edited Loading

Codecov Report

matthewfeickert left a comment

Choose a reason for hiding this comment

matthewfeickert left a comment

Choose a reason for hiding this comment

lukasheinrich commented Aug 2, 2020

alexander-held commented Aug 3, 2020

matthewfeickert commented Aug 3, 2020

kratsg commented Aug 3, 2020

alexander-held commented Aug 3, 2020

kratsg commented Jul 26, 2020 •

edited

Loading

codecov bot commented Jul 26, 2020 •

edited

Loading