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.

Sign up for GitHub

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

Jump to bottom

feat: support no POI #950

Merged
merged 18 commits into from
Jul 18, 2020
Merged

feat: support no POI #950

merged 18 commits into from
Jul 18, 2020

Conversation

lukasheinrich
Copy link
Contributor

@lukasheinrich lukasheinrich commented Jul 16, 2020 •
edited by kratsg
Loading

Description

closes #514, closes #946

* Allow POI-less models (poi_name = None)
* Raise exception when used in contexts that require POI

lukasheinrich
lukasheinrich commented Jul 16, 2020
View reviewed changes
src/pyhf/infer/mle.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 16, 2020 •
edited
Loading

Codecov Report

Merging #950 into master will increase coverage by 0.13%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #950      +/-   ##
==========================================
+ Coverage   96.31%   96.45%   +0.13%     
==========================================
  Files          57       57              
  Lines        3204     3212       +8     
  Branches      438      441       +3     
==========================================
+ Hits         3086     3098      +12     
+ Misses         75       73       -2     
+ Partials       43       41       -2
Flag Coverage Δ
#unittests 96.45% <100.00%> (+0.13%) ⬆️

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

Impacted Files Coverage Δ
src/pyhf/exceptions/__init__.py 100.00% <100.00%> (ø)
src/pyhf/infer/mle.py 100.00% <100.00%> (ø)
src/pyhf/infer/test_statistics.py 100.00% <100.00%> (ø)
src/pyhf/pdf.py 95.84% <100.00%> (+0.01%) ⬆️
src/pyhf/modifiers/normfactor.py 100.00% <0.00%> (+10.81%) ⬆️

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 0f99cc4...5287e54. Read the comment docs.

@lukasheinrich lukasheinrich changed the title support no POi feat: support no POI Jul 16, 2020
@lukasheinrich lukasheinrich requested review from kratsg and matthewfeickert and removed request for kratsg July 16, 2020 21:52
@matthewfeickert matthewfeickert added the feat/enhancement New feature or request label Jul 17, 2020
kratsg
kratsg requested changes Jul 17, 2020
View reviewed changes
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.

Some changes need to be made. Overall, looks surprisingly simple. Does pyhf inspect work with this?

src/pyhf/pdf.py Outdated Show resolved Hide resolved
matthewfeickert
matthewfeickert reviewed Jul 17, 2020
View reviewed changes
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.

Looking good. Just have some small questions and nitpicky suggestions.

src/pyhf/infer/mle.py Outdated Show resolved Hide resolved
src/pyhf/infer/mle.py Outdated Show resolved Hide resolved
src/pyhf/infer/test_statistics.py Outdated Show resolved Hide resolved
tests/test_pdf.py Outdated Show resolved Hide resolved
tests/test_pdf.py Outdated Show resolved Hide resolved
lukasheinrich and others added 3 commits July 17, 2020 15:34
@lukasheinrich
go
446c880
@lukasheinrich @matthewfeickert
Apply suggestions from code review
dbf973f 
Co-authored-by: Matthew Feickert <matthew.feickert@cern.ch>
@lukasheinrich
ok
3f01c31
matthewfeickert
matthewfeickert approved these changes Jul 17, 2020
View reviewed changes
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.

Just a few style suggestions for me, but I'll approve this. 👍

tests/test_pdf.py Outdated Show resolved Hide resolved
tests/test_pdf.py Outdated Show resolved Hide resolved
tests/test_pdf.py Outdated Show resolved Hide resolved
@matthewfeickert matthewfeickert added the API Changes the public API label Jul 17, 2020
@kratsg @matthewfeickert
Apply suggestions from code review
0380a30 
Co-authored-by: Matthew Feickert <matthew.feickert@cern.ch>
@kratsg kratsg merged commit afc2190 into master Jul 18, 2020
@kratsg kratsg deleted the nopoi branch July 18, 2020 17:42
@kratsg kratsg mentioned this pull request Oct 14, 2021
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewfeickert matthewfeickert matthewfeickert approved these changes

@kratsg kratsg kratsg approved these changes

Assignees

@lukasheinrich lukasheinrich

Labels
API Changes the public API feat/enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

allow poi-less Models background-only workspaces cannot be built as models
3 participants
@lukasheinrich @kratsg @matthewfeickert