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: Add pyhf.workspace.build #1101

Merged
merged 28 commits into from
Oct 14, 2020
Merged

feat: Add pyhf.workspace.build #1101

merged 28 commits into from
Oct 14, 2020

Conversation

lukasheinrich
Copy link
Contributor

@lukasheinrich lukasheinrich commented Oct 13, 2020

Description

model = pyhf.simplemodels.hepdata_like(
        signal_data=[12.0, 11.0], bkg_data=[50.0, 52.0], bkg_uncerts=[3.0, 7.0]
)
data = [51, 48] + model.config.auxdata
workspace = pyhf.workspace.build(model, data)

this is the inverse of

w = pyf.Workspace(foo)
model = w.model()
data = w.data(model)

maybe interesting for @alexander-held, too

ReadTheDocs build: https://pyhf.readthedocs.io/en/workspace_maker/_generated/pyhf.workspace.Workspace.html

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 workspace class method to build workspaces from a model and data
* Add test to validate build behavior

workspace = copy.deepcopy(model.spec)
workspace['version'] = '1.0.0'
workspace['measurements'] = [
{'name': name, 'config': {'poi': model.config.poi_name, 'parameters': []}}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this ignores fixed parameters. @kratsg maybe we add it in a follow-up PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukasheinrich I haven't looked at this much yet, but how much work would it take to get this all in one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something like this

param_configs = [{'name': param_name, 'inits': model.config.suggested_init()[model.config.param_set(param_name)['slice']], 'bounds': model.config.suggested_bounds()[model.config.param_set(param_name)['slice']), 'fixed': True} for param_name in model.config.parameters if model.config.suggested_fixed()[.....]]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@codecov
Copy link

codecov bot commented Oct 13, 2020

Codecov Report

Merging #1101 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1101      +/-   ##
==========================================
+ Coverage   96.87%   96.89%   +0.01%     
==========================================
  Files          62       62              
  Lines        3556     3570      +14     
  Branches      510      513       +3     
==========================================
+ Hits         3445     3459      +14     
  Misses         68       68              
  Partials       43       43              
Flag Coverage Δ
#unittests 96.89% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
src/pyhf/exceptions/__init__.py 100.00% <100.00%> (ø)
src/pyhf/mixins.py 100.00% <100.00%> (ø)
src/pyhf/utils.py 96.15% <100.00%> (ø)
src/pyhf/workspace.py 98.09% <100.00%> (+0.06%) ⬆️

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 c1727ce...ac5e2a4. Read the comment docs.

@lukasheinrich
Copy link
Contributor Author

black seems happy on my machine, but not in CI, @matthewfeickert any idea?

@matthewfeickert
Copy link
Member

black seems happy on my machine, but not in CI, @matthewfeickert any idea?

Where is black unhappy? It isn't complaining in the CI as the Lint workflow is passing.

@matthewfeickert matthewfeickert changed the title pyhf.workspace.build feat: Add pyhf.workspace.build Oct 13, 2020
@matthewfeickert matthewfeickert added the API Changes the public API label Oct 13, 2020
src/pyhf/workspace.py Outdated Show resolved Hide resolved
src/pyhf/workspace.py Outdated Show resolved Hide resolved
src/pyhf/workspace.py Outdated Show resolved Hide resolved
tests/test_workspace.py Show resolved Hide resolved
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.

This is looking good @lukasheinrich. 👍 I rebased it and fixed a few small things that got dropped in the merges and added in some things missing to the docstring.

This is basically done, but I agree with @kratsg thought that

but we should check a "roundtrip" of sorts with simpler workspaces

src/pyhf/workspace.py Outdated Show resolved Hide resolved
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 just have a formatting suggestion and then I agree with @kratsg's suggestion to only add in non-default settings.

src/pyhf/exceptions/__init__.py Outdated Show resolved Hide resolved
@matthewfeickert matthewfeickert added the tests pytest label Oct 14, 2020
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.

Thanks for this @lukasheinrich. 👍

@lukasheinrich lukasheinrich merged commit 6d4eb4e into master Oct 14, 2020
@lukasheinrich lukasheinrich deleted the workspace_maker branch October 14, 2020 22:21
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.

3 participants