Skip to content

Conversation

@kratsg
Copy link
Contributor

@kratsg kratsg commented Oct 13, 2021

Pull Request Description

No associated issues. Primarily drop the poi_name functionality in pyhf.Workspace.get_measurement as it is not really used and is kind of a bit weird anyway. Instead, allow for users to specify poi_name in pyhf.Workspace.model() in order to override the suggested POI that comes from the measurement they would like to use. This allows a user to use an existing measurement, but override the poi_name.

For example, pyhf.Workspace.model(poi_name=None) will allow for background-only models to be created from pyhf.Workspace which was not something that was allowed in the past.

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
* Remove `poi_name` functionality in `pyhf.Workspace.get_measurement`
* Allow for `poi_name` to be optionally set in `pyhf.Workspace.model` to override suggested POI
* Add tests for setting poi_name through `pyhf.Workspace.model`
* Provide intersphinx for jsonpatch docs

@kratsg kratsg added API Changes the public API feat/enhancement New feature or request fix A bug fix labels Oct 13, 2021
@kratsg kratsg self-assigned this Oct 13, 2021
@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #1636 (843a11b) into master (6f4ca1a) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1636      +/-   ##
==========================================
+ Coverage   97.99%   98.03%   +0.04%     
==========================================
  Files          63       63              
  Lines        4132     4128       -4     
  Branches      567      566       -1     
==========================================
- Hits         4049     4047       -2     
+ Misses         49       48       -1     
+ Partials       34       33       -1     
Flag Coverage Δ
contrib 25.00% <28.57%> (+0.02%) ⬆️
unittests 97.81% <100.00%> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
src/pyhf/workspace.py 100.00% <100.00%> (ø)
src/pyhf/pdf.py 97.69% <0.00%> (+0.65%) ⬆️

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 6f4ca1a...843a11b. Read the comment docs.

@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.

@kratsg This is nice. :)

I have two render changes and then a docstring length suggestion, but I like this.

@matthewfeickert matthewfeickert added the docs Documentation related label Oct 14, 2021
@alexander-held
Copy link
Member

related: #1602

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.

LGTM. 👍 Thanks @kratsg!

@kratsg kratsg merged commit 47b87db into master Oct 15, 2021
@kratsg kratsg deleted the feat/workspace-poiless branch October 15, 2021 11:00
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 fix A bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants