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: Rename kwarg with_aux to include_auxdata in pyhf.Workspace.data API #1562

Merged
merged 6 commits into from
Aug 26, 2021

Conversation

kratsg
Copy link
Contributor

@kratsg kratsg commented Aug 25, 2021

Pull Request Description

Closes #1561. This harmonizes the API a little more by renaming with_aux to include_auxdata in ws.data() calls to match model.expected_data().

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
* Rename keyword argument in pyhf.Workspace.data with_aux to include_auxdata
  - Harmonize the API between workspace and model
  - Possible breaking API change with pyhf v0.6.2

@kratsg kratsg added API Changes the public API feat/enhancement New feature or request user request Request coming form a pyhf user labels Aug 25, 2021
@kratsg kratsg self-assigned this Aug 25, 2021
@codecov
Copy link

codecov bot commented Aug 25, 2021

Codecov Report

Merging #1562 (47947cc) into master (29e15c2) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1562   +/-   ##
=======================================
  Coverage   97.70%   97.70%           
=======================================
  Files          63       63           
  Lines        4050     4050           
  Branches      576      576           
=======================================
  Hits         3957     3957           
  Misses         54       54           
  Partials       39       39           
Flag Coverage Δ
contrib 25.40% <50.00%> (ø)
unittests 97.48% <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%> (ø)

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 29e15c2...47947cc. Read the comment docs.

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.

I'd suggest not making the change that CodeFactor suggests as it slows things down for no benefit, but other than that this all looks good. I know that CodeFactor will complain but we can manually ignore it.

I guess this PR is technically API breaking but only if people are actively using the kwarg so I'm not really expecting this to break people. 👍

tests/test_workspace.py Outdated Show resolved Hide resolved
@alexander-held
Copy link
Member

Thanks for addressing this so quickly! It's a breaking change for cabinetry, but with this going into v0.6.3 (which introduces another breaking change anyway), that's not an issue at all from cabinetry's side.

kratsg and others added 2 commits August 26, 2021 07:54
Co-authored-by: Matthew Feickert <matthew.feickert@cern.ch>
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.

:shipit:

@matthewfeickert matthewfeickert merged commit 40bd916 into master Aug 26, 2021
@matthewfeickert matthewfeickert deleted the feat/harmonizeAPIAuxData branch August 26, 2021 13:41
matthewfeickert added a commit that referenced this pull request Sep 4, 2021
…ace.data` (#1588)

* Use `include_auxdata` kwarg for `pyhf.Workspace.data` as part of v0.6.3 API change
   - Amends PR #1562
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 user request Request coming form a pyhf user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auxdata kwarg harmonization in expected / observed data
3 participants