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

fix: Step sizes for fixed parameters in minuit should be 0.0 #1054

Merged
merged 3 commits into from
Sep 5, 2020

Conversation

kratsg
Copy link
Contributor

@kratsg kratsg commented Sep 4, 2020

Description

This resolves a comment made in #1051 where step sizes were not being zero'd out correctly.

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.

Originally posted by @alexander-held in #1051 (comment)

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
* Step sizes are set to 0.0 for fixed parameters in a fit for minuit
   - The step size acts as the uncertainty for fixed parameters
* Add test for step sizes for fixed parameters

@kratsg kratsg added the fix A bug fix label Sep 4, 2020
@kratsg kratsg self-assigned this Sep 4, 2020
@codecov
Copy link

codecov bot commented Sep 4, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1054   +/-   ##
=======================================
  Coverage   96.78%   96.78%           
=======================================
  Files          59       59           
  Lines        3388     3389    +1     
  Branches      489      489           
=======================================
+ Hits         3279     3280    +1     
  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/optimize/opt_minuit.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 0f094a4...c567cd0. 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.

LGTM. Thanks @kratsg. @alexander-held let us know if you have any comments here.

@matthewfeickert matthewfeickert added the tests pytest label Sep 5, 2020
@alexander-held
Copy link
Member

Thanks for including this! I rebased it into #1051 to test and it worked as expected.

@kratsg kratsg merged commit 81640a2 into master Sep 5, 2020
@kratsg kratsg deleted the fix/minuitFixedUncertainties branch September 5, 2020 11:52
kratsg added a commit that referenced this pull request Sep 5, 2020
* Step sizes are set to 0.0 for fixed parameters in a fit for minuit
   - The step size acts as the uncertainty for fixed parameters
* Add test for step sizes for fixed parameters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix A bug fix tests pytest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants