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: Make par_names a _ModelConfig property attribute #2027

Merged
merged 4 commits into from
Sep 23, 2022
Merged

feat: Make par_names a _ModelConfig property attribute #2027

merged 4 commits into from
Sep 23, 2022

Conversation

matthewfeickert
Copy link
Member

@matthewfeickert matthewfeickert commented Sep 23, 2022 •
edited
Loading

Description

Resolves #1700

Make pyhf.pdf._ModelConfig.par_names() a property attribute to match the pyhf.pdf._ModelConfig.par_order API.

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
* Make pyhf.pdf._ModelConfig.par_names() property attribute pyhf.pdf._ModelConfig.par_names,
  like the the pyhf.pdf._ModelConfig.par_order API.
* Update all usage of `.par_names()` to `par_names`.
* Use unittest.mock.PropertyMock to properly mock par_names return.

@matthewfeickert
feat: Make par_names a _ModelConfig property attribute
64f86ac 
* Make par_names() a property attribute to match the par_names API.
@matthewfeickert
Update tests to use par_name
f6a0348
@matthewfeickert matthewfeickert added feat/enhancement New feature or request tests pytest API Changes the public API labels Sep 23, 2022
@matthewfeickert matthewfeickert self-assigned this Sep 23, 2022
@matthewfeickert
Copy link
Member Author

@kratsg @lukasheinrich while adding the

>>> import pyhf
>>> model = pyhf.simplemodels.correlated_background(
...     signal=[12.0, 11.0],
...     bkg=[50.0, 52.0],
...     bkg_up=[45.0, 57.0],
...     bkg_down=[55.0, 47.0],
... )
>>> model.config.par_order
['correlated_bkg_uncertainty', 'mu']
>>> model.config.par_names()
['correlated_bkg_uncertainty', 'mu']

example to PR #1705 I noted there's a bit of an inconsistency here in API. Can we slip this in under the wire for v0.7.0?

@matthewfeickert
Copy link
Member Author

@alexander-held This will hit cabinetry but can be easily patched with

git grep --name-only "par_names()" -- src/ | xargs sed -i 's/par_names()/par_names/g'

@alexander-held
Copy link
Member

See #1700, I assume that will still go into 0.7?

@matthewfeickert
Copy link
Member Author

See #1700

Nice. 😎

I assume that will still go into 0.7?

Unless people object to be messing around with the API this late. 😬

@matthewfeickert
Copy link
Member Author

matthewfeickert commented Sep 23, 2022 •
edited
Loading

CI is failing as I forgot that because of PR #1536

pyhf/tests/test_optim.py

Lines 579 to 582 in cccbda1

pdf.config.par_names = mocker.Mock(return_value=None)
_, result = pyhf.infer.mle.fit(data, pdf, return_result_obj=True)
assert 'minuit' in result
assert result.minuit.parameters == ('x0', 'x1')

needs to get fixes now too. I think this requires use of unittest.mock.PropertyMock.

@codecov
Copy link

codecov bot commented Sep 23, 2022 •
edited
Loading

Codecov Report

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

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

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #2027   +/-   ##
=======================================
  Coverage   98.29%   98.30%           
=======================================
  Files          69       69           
  Lines        4529     4530    +1     
  Branches      738      738           
=======================================
+ Hits         4452     4453    +1     
  Misses         45       45           
  Partials       32       32
Flag Coverage Δ
contrib 27.52% <50.00%> (+0.01%) ⬆️
doctest 61.14% <100.00%> (+<0.01%) ⬆️
unittests-3.10 96.31% <100.00%> (+<0.01%) ⬆️
unittests-3.7 96.28% <100.00%> (ø)
unittests-3.8 96.33% <100.00%> (+<0.01%) ⬆️
unittests-3.9 96.35% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
src/pyhf/optimize/mixins.py 100.00% <100.00%> (ø)
src/pyhf/pdf.py 98.03% <100.00%> (+<0.01%) ⬆️

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 mentioned this pull request Sep 23, 2022
Merged
4 tasks
@matthewfeickert
Copy link
Member Author

Relevant RTD build: https://pyhf--2027.org.readthedocs.build/en/2027/_generated/pyhf.pdf._ModelConfig.html#pyhf.pdf._ModelConfig.par_names

@kratsg kratsg merged commit bc5cf85 into main Sep 23, 2022
@kratsg kratsg deleted the refactor/make-par_names-attribute branch September 23, 2022 21:38
This was referenced Sep 25, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kratsg kratsg kratsg approved these changes

@lukasheinrich lukasheinrich Awaiting requested review from lukasheinrich

Assignees

@matthewfeickert matthewfeickert

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

Successfully merging this pull request may close these issues.

Make model.config.par_names() a property?
3 participants
@matthewfeickert @alexander-held @kratsg