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

feat: Support configuring fixed values automatically from Model #1051

Merged
merged 24 commits into from
Sep 7, 2020

Conversation

kratsg
Copy link
Contributor

@kratsg kratsg commented Sep 3, 2020

Description

Replaces #846.

Provides support for automatically passing through the fixed parameters from the model configuration into all of the minimization/inference that we do in the pyhf code-base, namely:

  • pyhf.infer.hypotest
  • pyhf.infer.calculators.generate_asimov_data
  • pyhf.infer.calculators.AsymptoticCalculators
  • pyhf.infer.mle.fit
  • pyhf.infer.mle.fixed_poi_fit
  • pyhf.infer.test_statistics._qmu_like
  • pyhf.infer.test_statistics._tmu_like
  • pyhf.infer.test_statistics.qmu
  • pyhf.infer.test_statistics.qmu_tilde
  • pyhf.infer.test_statistics.tmu
  • pyhf.infer.test_statistics.tmu_tilde

We will pass in fixed_params all the way through to the optimization which will stitch together the two if need-be.

This is a two-fold feature that needs to be refactored/be more elegant a bit later. fixed_vals are things that are a list of tuples like so: [(parameter_index, parameter_value), ...]. fixed_params are what comes from the pdf.config.suggested_fixed().

  • in cases where fixed_params is optional, the model fixed parameters are retrieved
  • in cases where fixed_params is not optional, nothing is done (a pass-through)

This tries to model the behavior we have identically for init_pars and par_bounds everywhere.

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 support for automatically passing through fixed parameters from the model configuration into all of the minimization/inference API
* Add tests for fixed values

Co-authored-by: Carlo Gottardo <[email protected]>

@kratsg kratsg added the feat/enhancement New feature or request label Sep 3, 2020
@kratsg kratsg marked this pull request as ready for review September 4, 2020 00:01
@kratsg kratsg mentioned this pull request Sep 4, 2020
4 tasks
@codecov
Copy link

codecov bot commented Sep 4, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1051   +/-   ##
=======================================
  Coverage   96.78%   96.78%           
=======================================
  Files          59       59           
  Lines        3389     3394    +5     
  Branches      489      490    +1     
=======================================
+ Hits         3280     3285    +5     
  Misses         68       68           
  Partials       41       41           
Flag Coverage Δ
#unittests 96.78% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
src/pyhf/infer/__init__.py 100.00% <100.00%> (ø)
src/pyhf/infer/calculators.py 100.00% <100.00%> (ø)
src/pyhf/infer/mle.py 100.00% <100.00%> (ø)
src/pyhf/infer/test_statistics.py 100.00% <100.00%> (ø)

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 516ee1b...3364a79. Read the comment docs.

Copy link
Member

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kratsg Core wise this is great. Thanks for stepping up and knocking this out this week. Just some readability suggestions and questions for the most part.

src/pyhf/infer/__init__.py Outdated Show resolved Hide resolved
src/pyhf/infer/__init__.py Show resolved Hide resolved
src/pyhf/infer/calculators.py Outdated Show resolved Hide resolved
src/pyhf/infer/calculators.py Outdated Show resolved Hide resolved
src/pyhf/infer/calculators.py Outdated Show resolved Hide resolved
src/pyhf/infer/test_statistics.py Outdated Show resolved Hide resolved
src/pyhf/infer/test_statistics.py Outdated Show resolved Hide resolved
src/pyhf/infer/test_statistics.py Outdated Show resolved Hide resolved
src/pyhf/infer/test_statistics.py Outdated Show resolved Hide resolved
tests/test_teststats.py Outdated Show resolved Hide resolved
@matthewfeickert matthewfeickert added API Changes the public API tests pytest bumpversion/patch Create a patch version release labels Sep 4, 2020
@alexander-held
Copy link
Member

alexander-held commented Sep 4, 2020

Suggestion: For the minuit optimizer, the initial step size is set in minuit_optimizer._get_minimizer. For a fixed Gaussian-constrained nuisance parameter with default bounds [-5, 5], this is 0.01 with the default 1000 steps. This will be the parameter uncertainty reported by minuit after the fit. Given that it does not make sense to have an uncertainty for a fixed parameter, it might be nice to set the entries of step_sizes corresponding to fixed parameters to 0 instead.

Copy link
Member

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kratsg LGTM for the original scope. As this is a release PR if we can get @lukasheinrich's review as well that would be good.

I haven't looked at @alexander-held's suggestion in terms of scope yet, so should we try to get that in here or is that beyond scope and should be its own Issue + PR? Also, would be good to understand how the changes in 0ad8a7d managed to break everything if there's time. :?

@matthewfeickert matthewfeickert added bumpversion/patch Create a patch version release and removed bumpversion/patch Create a patch version release labels Sep 4, 2020
@scikit-hep scikit-hep deleted a comment from github-actions bot Sep 4, 2020
@kratsg kratsg force-pushed the cgottard/fixed_vals branch from ff171ca to a947035 Compare September 4, 2020 22:16
@kratsg kratsg force-pushed the cgottard/fixed_vals branch from a947035 to 8a7c992 Compare September 5, 2020 11:53
@kratsg
Copy link
Contributor Author

kratsg commented Sep 5, 2020

Suggestion: For the minuit optimizer, the initial step size is set in minuit_optimizer._get_minimizer. For a fixed Gaussian-constrained nuisance parameter with default bounds [-5, 5], this is 0.01 with the default 1000 steps. This will be the parameter uncertainty reported by minuit after the fit. Given that it does not make sense to have an uncertainty for a fixed parameter, it might be nice to set the entries of step_sizes corresponding to fixed parameters to 0 instead.

This is done in #1054.

@kratsg kratsg force-pushed the cgottard/fixed_vals branch 2 times, most recently from fb6b303 to d465637 Compare September 5, 2020 12:07
@scikit-hep scikit-hep deleted a comment from github-actions bot Sep 5, 2020
@matthewfeickert matthewfeickert added bumpversion/patch Create a patch version release and removed bumpversion/patch Create a patch version release labels Sep 5, 2020
@github-actions
Copy link

github-actions bot commented Sep 5, 2020

I've queued this up. When it gets merged, I'll create a patch release from v0.5.1 → v0.5.2 which includes the following 19 change(s) [including this PR]:

  • feat: Support configuring fixed values automatically from Model
  • docs: Add SModelS paper to citations list
  • fix: Step sizes for fixed parameters in minuit should be 0.0
  • fix: Sync SciPy minimizer initialized parameter values with fixed values
  • feat: Prune/Rename raise errors if item name not in workspace
  • style: Update to Black v20.8 formatting
  • fix: Tensor conversion for expected data in _MainModel
  • feat: Add pyhf.Workspace.sorted to give ability to sort Workspace
  • fix: Make pyhf.Workspace.combine return cls instead of Workspace
  • fix: Tensor conversion for expected aux and actual data
  • fix: Weakly reference callables for pub-sub model
  • docs: Add learn notebook on test statistics
  • docs: Extend infer docstrings with mathematical examples
  • refactor: Make hypotest return CLs as 0-d tensor
  • feat: Add return_by_sample to expected_data API
  • fix: Tensorflow tile tensor backend feature parity
  • docs: Add NSF award badge for Throughput project tracking
  • docs: Make all examples in README full examples
  • docs: Add citation in Higgs boson potential at colliders paper

  • If you make any more changes, you probably want to re-trigger me again by removing the bumpversion/patch label and then adding it back again.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    API Changes the public API bumpversion/patch Create a patch version release feat/enhancement New feature or request tests pytest
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    5 participants