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: Validate workspace specification before accessing parts of it #1953

Merged

Conversation

alexander-held
Copy link
Member

@alexander-held alexander-held commented Aug 18, 2022

Description

The validation of a workspace specification against the schema currently takes place after some aspects of the workspace are already accessed in the constructor of _ChannelSummaryMixin. This can lead to unintuitive error messages, which should instead be caught by the schema validation. This PR re-orders the logic such that validation takes place first, ensuring that the relevant information is available for subsequent steps (assuming validation succeeds).

resolves #1952

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
* Validate the workspace specification in Model construction before any spec information
  can be accessed in the constructor of _ChannelSummaryMixin.
* Add test that checks for an pyhf.exceptions.InvalidSpecification being raised, indicating
  that the invalid model spec has been caught before a KeyError would be raised later.

@alexander-held alexander-held marked this pull request as draft August 18, 2022 18:36
@codecov
Copy link

codecov bot commented Aug 18, 2022

Codecov Report

Merging #1953 (90e82a2) into master (fd589ef) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1953   +/-   ##
=======================================
  Coverage   98.26%   98.26%           
=======================================
  Files          69       69           
  Lines        4439     4439           
  Branches      748      748           
=======================================
  Hits         4362     4362           
  Misses         45       45           
  Partials       32       32           
Flag Coverage Δ
contrib 25.81% <0.00%> (ø)
doctest 61.02% <100.00%> (ø)
unittests-3.10 96.14% <100.00%> (ø)
unittests-3.7 96.13% <100.00%> (ø)
unittests-3.8 96.17% <100.00%> (ø)
unittests-3.9 96.19% <100.00%> (ø)

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%> (ø)

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

@alexander-held alexander-held marked this pull request as ready for review August 19, 2022 06:17
@matthewfeickert matthewfeickert added the tests pytest label Aug 22, 2022
@matthewfeickert
Copy link
Member

Thanks @alexander-held! We very much appreciate your contributions. :)

@matthewfeickert matthewfeickert changed the title feat: validate workspace specification before accessing parts of it feat: Validate workspace specification before accessing parts of it Aug 22, 2022
@matthewfeickert matthewfeickert merged commit 82bd0f7 into scikit-hep:master Aug 22, 2022
@alexander-held alexander-held deleted the feat/validate-workspace-first branch August 22, 2022 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat/enhancement New feature or request tests pytest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Model specification is accessed before being validated
3 participants