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

docs: Clarify staterror modifier specification #1856

Merged
merged 12 commits into from
May 24, 2022

Conversation

Moelf
Copy link
Contributor

@Moelf Moelf commented Apr 29, 2022

clarify and fix typo for Staterror modifier.

Description

The \delta parameter is the absolute error in each bin due to MC Stat at +/- 1 sigma. Thus, the \sigma of the Gaussian constraint term should have \delta / nominal value not the other way around.

And by play around with the logpdf() output, I'm convinced that the input in JSON is \delta rather than \delta^2 (https://scikit-hep.org/pyhf/intro.html#id25)

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
* Correct the description of the calculation of the scale of the constraint
to indicate it is calculated from the relative statistical uncertainty from the
channel's event rate.
* Add additional channel subscripts to make the notation be more explicit that
staterror is per-channel.
* Remove typo of additional 'are'.
* Add Jerry Ling to contributor list.

@Moelf Moelf changed the title Update likelihood.rst docs: Update likelihood.rst Apr 29, 2022
@codecov
Copy link

codecov bot commented Apr 29, 2022

Codecov Report

Merging #1856 (a8eb612) into master (de4ce07) will not change coverage.
The diff coverage is n/a.

❗ Current head a8eb612 differs from pull request most recent head ef98feb. Consider uploading reports for the commit ef98feb to get more accurate results

@@           Coverage Diff           @@
##           master    #1856   +/-   ##
=======================================
  Coverage   98.16%   98.16%           
=======================================
  Files          68       68           
  Lines        4315     4315           
  Branches      725      725           
=======================================
  Hits         4236     4236           
  Misses         46       46           
  Partials       33       33           
Flag Coverage Δ
contrib 26.44% <0.00%> (ø)
doctest 60.78% <0.00%> (ø)
unittests-3.10 96.03% <0.00%> (ø)
unittests-3.7 96.02% <0.00%> (ø)

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


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 b4abe41...ef98feb. Read the comment docs.

@matthewfeickert
Copy link
Member

Thanks for the PR @Moelf. We'll try to get to it before the end of the weekend.

As you didn't make a GitHub Issue for this can you give a full summary in the PR body and then also finish off the rest of the CONTRIBUTING guidelines (hint: adding yourself to contributors. 😉)?

@Moelf
Copy link
Contributor Author

Moelf commented Apr 29, 2022

I think the \delta^2 in the Table can be understood as:

  1. we take the quadrature sum of \delta_b across all the samples with the same MC error
  2. we pass the sqrt(sum) into the staterror constructor.

So I think the table is not wrong.

@matthewfeickert
Copy link
Member

Note to myself for when I review this tomorrow: Relevant RTD build section is here: https://pyhf--1856.org.readthedocs.build/en/1856/likelihood.html#mc-statistical-uncertainty-staterror

@matthewfeickert matthewfeickert changed the title docs: Update likelihood.rst docs: Clarify staterror modifier specification May 4, 2022
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.

@lukasheinrich @kratsg while I think we should maybe discuss more the notational consistency across the docs I don't think that this needs to hold up Jerry's PR. So if we get approvals from both of you then I this LGTM for now.

@matthewfeickert matthewfeickert force-pushed the patch-1 branch 2 times, most recently from d5a214f to 24a8d4c Compare May 10, 2022 08:12
@matthewfeickert
Copy link
Member

@kratsg @lukasheinrich if this can get reviewed today by one (ideally both) of you then I think we can merge this.

@matthewfeickert matthewfeickert merged commit 461d368 into scikit-hep:master May 24, 2022
@matthewfeickert
Copy link
Member

Thanks for your PR @Moelf!

@Moelf Moelf deleted the patch-1 branch May 24, 2022 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants