Skip to content

Conversation

@aryan26roy
Copy link
Contributor

@aryan26roy aryan26roy commented Sep 30, 2021

Description

This PR makes the test statistic configurable in intervals/upperlimit by adding a kwarg in the function definition.

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 hypotest_kwargs to pyhf.infer.intervals.upperlimit to allow passing in configuration options to the underlying hypotest call
* Add test for hypotest_kwargs use
* Add Aryan Roy to contributor list

:class:`~pyhf.infer.hypotest` results at each test POI.
Only returned when ``return_results`` is ``True``.
"""
test_stats = kwargs.pop('test_stat', "qtilde")
Copy link
Contributor

Choose a reason for hiding this comment

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

should we maybe rename kwargs into hypotest_kwargs and pass

hypotest(mu, data, model, return_expected_set=True, **hypotest_kwargs)

Copy link
Member

Choose a reason for hiding this comment

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

The idea being here that this allows for further manipulation of hypotest from upperlimit as desired? Seems to make sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lukasheinrich @matthewfeickert Alright I'll pass the hypotest_kwargs to infer/hypotest directly.

@matthewfeickert matthewfeickert changed the title feat: Added test_stat kwarg to upperlimit feat: Add hypotest kwargs to pyhf.infer.intervals.upperlimit Oct 11, 2021
@matthewfeickert matthewfeickert added API Changes the public API feat/enhancement New feature or request tests pytest labels Oct 11, 2021
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.

Thanks @aryan26roy (and sorry for the slow reply — as you can see all of the core devs have been swamped lately). This basically LGTM as the test covers the minimal case and I'm pushing a patch now, but can you also add yourself to the contributors list?

pyhf/CONTRIBUTING.md

Lines 42 to 44 in 7ecaa63

7. Make sure that you've added your name to `docs/contributors.rst`.
If you haven't **please** do so by simply appending your name to the bottom of the list.
We are thankful for and value your contributions to `pyhf`, not matter the size.

@matthewfeickert
Copy link
Member

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.

I'm going to approve this now, but we should probably get #1622 in first so that we can test that Codecov is properly getting coverage reports even from contributor PRs.

@codecov
Copy link

codecov bot commented Oct 12, 2021

Codecov Report

Merging #1613 (c7a126d) into master (54b44ad) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1613   +/-   ##
=======================================
  Coverage   97.70%   97.70%           
=======================================
  Files          63       63           
  Lines        4048     4048           
  Branches      576      576           
=======================================
  Hits         3955     3955           
  Misses         54       54           
  Partials       39       39           
Flag Coverage Δ
contrib 25.44% <100.00%> (ø)
unittests 97.48% <100.00%> (ø)

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

Impacted Files Coverage Δ
src/pyhf/infer/intervals.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 54b44ad...c7a126d. Read the comment docs.

@matthewfeickert
Copy link
Member

Okay PR #1622 is working as intended, so we now have Codecov reporting on contributors PRs. @lukasheinrich @kratsg this should be ready for review now.

@matthewfeickert matthewfeickert merged commit 9358f85 into scikit-hep:master Oct 12, 2021
@matthewfeickert
Copy link
Member

Thanks for the PR @aryan26roy!

@aryan26roy
Copy link
Contributor Author

@matthewfeickert , glad to be of help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API Changes the public API feat/enhancement New feature or request tests pytest

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pass calculator options through pyhf.infer.upperlimit (toys) Make test statistic configurable in infer.upperlimit

4 participants