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: per-channel yield uncertainty contribution from staterror-staterror terms #324

Merged
merged 2 commits into from
Feb 9, 2022

Conversation

alexander-held
Copy link
Member

@alexander-held alexander-held commented Feb 8, 2022

The model_utils.yield_stdev implementation contained an optimization for staterror-staterror cross terms, which have no contribution when calculating per-bin uncertainties:

if (
labels[i_par][0:10] == "staterror_"
and labels[j_par][0:10] == "staterror_"
):
continue # two different staterrors are orthogonal, no contribution

When the functionality to calculate per-channel uncertainties was added in #189, the optimization should have been updated, since these cross terms no longer cancel out for the per-channel calculation. This now fixes the bug in per-channel yield uncertainties by removing this performance optimization completely.

While this does make the calculation slower overall, that is not a concern since it will be replaced completely by #316 afterwards, resulting in a significant speed-up. This intermediate fix is just meant to split the development up into steps that are hopefully easier to follow when looking back at this in the future.

resolves #323

* fix calculation of per-channel yield uncertainties
* calculation was previously missing contributions from staterror-staterror terms

@codecov
Copy link

codecov bot commented Feb 8, 2022

Codecov Report

Merging #324 (d41065f) into master (f34c73a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #324   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        23           
  Lines         1889      1886    -3     
  Branches       306       305    -1     
=========================================
- Hits          1889      1886    -3     
Impacted Files Coverage Δ
src/cabinetry/model_utils.py 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 f34c73a...d41065f. Read the comment docs.

@alexander-held alexander-held merged commit ea3ee7d into master Feb 9, 2022
@alexander-held alexander-held deleted the fix/channel-yield-uncertainty-staterror branch February 9, 2022 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Per-channel uncertainty calculation misses contributions from staterror-staterror terms
1 participant