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: Promote validate kwarg to top-level functions in pyhf.simplemodels #1858

Merged

Conversation

phinate
Copy link
Contributor

@phinate phinate commented May 2, 2022

When doing experiments for differentiable models (#882), it's sometimes been easier to just skip validation with simplemodels, which I've been writing out specs for. This just reduces a bit of code overhead for me when I do that ;)

Made sure to highlight in the docstring that this should probably never be touched if one doesn't know why.

This is also partially because #1665 isn't fully done yet, which would probably make this redundant in some ways.

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 `validate` kwarg to pyhf.simplemodels.uncorrelated_background and
pyhf.simplemodels.correlated_background API. This allows expert users to avoid
validating their models in specific circumstances.
* Add Nathan Simpson to contributors list.

@kratsg
Copy link
Contributor

kratsg commented May 2, 2022

Since master allows for custom specs - why not use a custom "catch-all" spec like type: object and no other details?

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

codecov bot commented May 2, 2022

Codecov Report

Base: 98.24% // Head: 98.24% // No change to project coverage 👍

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

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1858   +/-   ##
=======================================
  Coverage   98.24%   98.24%           
=======================================
  Files          68       68           
  Lines        4378     4378           
  Branches      726      726           
=======================================
  Hits         4301     4301           
  Misses         45       45           
  Partials       32       32           
Flag Coverage Δ
contrib 26.58% <50.00%> (ø)
doctest 60.57% <100.00%> (ø)
unittests-3.10 96.13% <100.00%> (ø)
unittests-3.7 96.12% <100.00%> (ø)
unittests-3.8 96.16% <100.00%> (ø)
unittests-3.9 96.18% <100.00%> (ø)

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

Impacted Files Coverage Δ
src/pyhf/simplemodels.py 92.30% <100.00%> (ø)

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.

@kratsg kratsg changed the title Push validate kwarg up to the top-level functions in pyhf.simplemodels feat: Promote validate kwarg to top-level functions in pyhf.simplemodels Aug 30, 2022
@kratsg kratsg closed this Aug 31, 2022
@kratsg kratsg reopened this Aug 31, 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 the PR and for your contributions @phinate (also thanks for your patience as this one got put off for some time)!

@matthewfeickert matthewfeickert added the docs Documentation related label Sep 2, 2022
@matthewfeickert matthewfeickert merged commit e366eb9 into scikit-hep:master Sep 2, 2022
@phinate phinate deleted the feat/simplemodels-validate branch September 3, 2022 10:33
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.

None yet

3 participants