Skip to content

Conversation

@kratsg
Copy link
Contributor

@kratsg kratsg commented Sep 5, 2022

Pull Request Description

Resolves #1984 by allowing _ModelConfig.set_poi(None) to be allowed behavior to remove the POI from a model.

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
* Allow for `_ModelConfig.set_poi(None)` as a way to unset the POI from a model.
* Add test of `model.config.set_poi` for value of the poi that are valid strings and None.

@kratsg kratsg added API Changes the public API feat/enhancement New feature or request labels Sep 5, 2022
@kratsg kratsg self-assigned this Sep 5, 2022
@codecov
Copy link

codecov bot commented Sep 5, 2022

Codecov Report

Base: 98.25% // Head: 98.25% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (405a51c) compared to base (e4aae10).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1985   +/-   ##
=======================================
  Coverage   98.25%   98.25%           
=======================================
  Files          68       68           
  Lines        4416     4420    +4     
  Branches      726      727    +1     
=======================================
+ Hits         4339     4343    +4     
  Misses         45       45           
  Partials       32       32           
Flag Coverage Δ
contrib 26.94% <0.00%> (-0.03%) ⬇️
doctest 60.83% <0.00%> (-0.06%) ⬇️
unittests-3.10 96.17% <100.00%> (+<0.01%) ⬆️
unittests-3.7 96.15% <100.00%> (+<0.01%) ⬆️
unittests-3.8 96.19% <100.00%> (+<0.01%) ⬆️
unittests-3.9 96.22% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
src/pyhf/pdf.py 98.01% <100.00%> (+0.02%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@matthewfeickert matthewfeickert added the tests pytest label Sep 5, 2022
@matthewfeickert matthewfeickert changed the title feat: Add _ModelConfig.set_poi() to public api feat: Add support for _ModelConfig.set_poi(None) to unset model POI Sep 5, 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.

LGTM. 👍 Thanks for being so fast on this @kratsg and for the quick iteration @alexander-held.

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.

Can no longer set several model config attributes

3 participants