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: Add AsymptoticCalculator #750

Merged
merged 25 commits into from
Mar 4, 2020
Merged

feat: Add AsymptoticCalculator #750

merged 25 commits into from
Mar 4, 2020

Conversation

lukasheinrich
Copy link
Contributor

@lukasheinrich lukasheinrich commented Jan 25, 2020

Description

Another in the series of #709 .

This introduces the AsymptoticCalculator

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 an 'infer.calculators' API giving an asymptotic calculator and asymptotic test stat distribution
* Move calculator functions from 'infer.utils' to 'infer.calculators'

@lukasheinrich lukasheinrich changed the title Calcapi22 feat: AsymptoticCalculator Jan 25, 2020
@lgtm-com
Copy link

lgtm-com bot commented Jan 25, 2020

This pull request introduces 2 alerts when merging f8e6bda into a508732 - view on LGTM.com

new alerts:

  • 2 for Unused local variable

@codecov
Copy link

codecov bot commented Jan 25, 2020

Codecov Report

Merging #750 into master will increase coverage by <.01%.
The diff coverage is 98.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #750      +/-   ##
==========================================
+ Coverage   95.33%   95.33%   +<.01%     
==========================================
  Files          54       54              
  Lines        2982     3004      +22     
  Branches      425      426       +1     
==========================================
+ Hits         2843     2864      +21     
- Misses         94       95       +1     
  Partials       45       45
Flag Coverage Δ
#unittests 95.33% <98.41%> (ø) ⬆️
Impacted Files Coverage Δ
src/pyhf/infer/__init__.py 100% <100%> (ø) ⬆️
src/pyhf/infer/calculators.py 97.95% <97.95%> (ø)

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 e3f0eb3...2f828d3. Read the comment docs.

@lukasheinrich lukasheinrich mentioned this pull request Jan 25, 2020
4 tasks
@lukasheinrich lukasheinrich requested review from kratsg and matthewfeickert and removed request for kratsg January 25, 2020 22:59
@matthewfeickert matthewfeickert added the feat/enhancement New feature or request label Jan 26, 2020
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 think overall this is really nice, so thanks! No functional issues with the code from me, but it would be nice if the docstrings could be improved to match the style of the existing docs like the tensor backends. In addition to formatting changes it would also help if examples were added. I would offer to make these changes that I'm asking for, but I realistically won't get to them until after my talk is written (which will be Wednesday).

src/pyhf/infer/calculators.py Outdated Show resolved Hide resolved
src/pyhf/infer/__init__.py Outdated Show resolved Hide resolved
src/pyhf/infer/__init__.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/calculators.py Outdated Show resolved Hide resolved
@matthewfeickert matthewfeickert added the docs Documentation related label Jan 26, 2020
@lukasheinrich
Copy link
Contributor Author

@matthewfeickert if you can push your changes that'd be nice

@matthewfeickert
Copy link
Member

matthewfeickert commented Feb 1, 2020

While trying to get the docs to show the full signatures and docstrings:

.. autosummary::
   :toctree: _generated/
   :nosignatures:
   :template: modifierclass.rst

I'm getting the following warnings (which we treat as errors) when I try to build the docs:

WARNING: error while formatting arguments for pyhf.infer.calculators.generate_asimov_data: 'function' object has no attribute '__mro__'
WARNING: error while formatting arguments for pyhf.infer.hypotest: 'function' object has no attribute '__mro__'
WARNING: error while formatting arguments for pyhf.infer.mle.fit: 'function' object has no attribute '__mro__'
WARNING: error while formatting arguments for pyhf.infer.mle.fixed_poi_fit: 'function' object has no attribute '__mro__'
WARNING: error while formatting arguments for pyhf.infer.mle.twice_nll: 'function' object has no attribute '__mro__'
WARNING: error while formatting arguments for pyhf.infer.test_statistics.qmu: 'function' object has no attribute '__mro__'

which I believe is happening as __mro__ only exists on the class, and these functions exist in the source code outside of a class definition. Thoughts on how to avoid/fix this?

@lgtm-com
Copy link

lgtm-com bot commented Feb 11, 2020

This pull request introduces 1 alert and fixes 1 when merging ca360aa into 7c3ba2e - view on LGTM.com

new alerts:

  • 1 for Module imports itself

fixed alerts:

  • 1 for Module imports itself

@lgtm-com
Copy link

lgtm-com bot commented Feb 29, 2020

This pull request introduces 1 alert and fixes 1 when merging 09cd082 into aa25803 - view on LGTM.com

new alerts:

  • 1 for Module imports itself

fixed alerts:

  • 1 for Module imports itself

@lgtm-com
Copy link

lgtm-com bot commented Feb 29, 2020

This pull request introduces 1 alert and fixes 1 when merging 68d6a3c into aa25803 - view on LGTM.com

new alerts:

  • 1 for Module imports itself

fixed alerts:

  • 1 for Module imports itself

@matthewfeickert matthewfeickert changed the title feat: AsymptoticCalculator feat: Add AsymptoticCalculator Feb 29, 2020
@matthewfeickert
Copy link
Member

I think since the problem with the docs is roughly documented in Issue #761 that we can just go ahead and review this.

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.

This LGTM now. Thanks @lukasheinrich for both the useful PR and for your extreme patience with how long this took to review again.

Copy link
Contributor

@kratsg kratsg left a comment

Choose a reason for hiding this comment

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

Overall, I like the refactoring. There's still more to digest here and clean-up.

src/pyhf/infer/__init__.py Outdated Show resolved Hide resolved
src/pyhf/infer/__init__.py Show resolved Hide resolved
src/pyhf/infer/calculators.py Show resolved Hide resolved
src/pyhf/infer/calculators.py Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Mar 4, 2020

This pull request introduces 1 alert and fixes 1 when merging 9a9d6ef into e3f0eb3 - view on LGTM.com

new alerts:

  • 1 for Module imports itself

fixed alerts:

  • 1 for Module imports itself

@lgtm-com
Copy link

lgtm-com bot commented Mar 4, 2020

This pull request introduces 1 alert and fixes 1 when merging 9c69fb5 into e3f0eb3 - view on LGTM.com

new alerts:

  • 1 for Module imports itself

fixed alerts:

  • 1 for Module imports itself

@lgtm-com
Copy link

lgtm-com bot commented Mar 4, 2020

This pull request introduces 1 alert and fixes 1 when merging 2f828d3 into e3f0eb3 - view on LGTM.com

new alerts:

  • 1 for Module imports itself

fixed alerts:

  • 1 for Module imports itself

@kratsg kratsg added the API Changes the public API label Mar 4, 2020
@kratsg kratsg merged commit 73da08d into master Mar 4, 2020
@kratsg kratsg deleted the calcapi22 branch March 4, 2020 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Changes the public API docs Documentation related feat/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants