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

refactor: Create infer subpackage to hold our statistics code #531

Merged
merged 16 commits into from
Dec 9, 2019
Merged

refactor: Create infer subpackage to hold our statistics code #531

merged 16 commits into from
Dec 9, 2019

Conversation

kratsg
Copy link
Contributor

@kratsg kratsg commented Aug 26, 2019 •
edited by matthewfeickert
Loading

Description

This will also be a bumpversion/minor update.

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
* Create a pyhf.infer subpackage
* Move cli.stats to cli.infer
* Bump version: 0.2.2 → 0.3.0

@matthewfeickert matthewfeickert mentioned this pull request Sep 10, 2019
Open
41 tasks
@matthewfeickert matthewfeickert added the feat/enhancement New feature or request label Sep 26, 2019
@kratsg kratsg mentioned this pull request Oct 9, 2019
Open
@lukasheinrich lukasheinrich mentioned this pull request Oct 14, 2019
Closed
@kratsg kratsg force-pushed the feature/statsSubpackage branch 2 times, most recently from 9ca3e22 to 302d41d Compare December 6, 2019 18:59
@kratsg kratsg changed the title Feature/stats subpackage refactor: Create stats subpackage to move our statistics into one location Dec 6, 2019
@kratsg kratsg marked this pull request as ready for review December 6, 2019 19:02
@codecov
Copy link

codecov bot commented Dec 6, 2019 •
edited
Loading

Codecov Report

Merging #531 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #531      +/-   ##
==========================================
+ Coverage   95.11%   95.12%   +0.01%     
==========================================
  Files          47       50       +3     
  Lines        2722     2730       +8     
  Branches      380      380              
==========================================
+ Hits         2589     2597       +8     
  Misses         88       88              
  Partials       45       45
Flag Coverage Δ
#unittests 95.12% <100%> (+0.01%) ⬆️
Impacted Files Coverage Δ
src/pyhf/utils.py 92.68% <ø> (-4.54%) ⬇️
src/pyhf/cli/infer.py 97.72% <100%> (ø)
src/pyhf/cli/__init__.py 100% <100%> (ø) ⬆️
src/pyhf/cli/cli.py 100% <100%> (ø) ⬆️
src/pyhf/infer/__init__.py 100% <100%> (ø)
src/pyhf/version.py 100% <100%> (ø) ⬆️
src/pyhf/infer/test_statistics.py 100% <100%> (ø)
src/pyhf/infer/utils.py 100% <100%> (ø)
src/pyhf/__init__.py 96.96% <100%> (+0.09%) ⬆️
... and 2 more

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 1347b96...ad1aa13. Read the comment docs.

@kratsg kratsg self-assigned this Dec 6, 2019
@kratsg kratsg added the refactor A code change that neither fixes a bug nor adds a feature label Dec 6, 2019
@kratsg kratsg requested review from matthewfeickert and lukasheinrich and removed request for matthewfeickert December 8, 2019 20:09
@matthewfeickert matthewfeickert force-pushed the feature/statsSubpackage branch from 4733f03 to 4a584ab Compare December 9, 2019 02:58
matthewfeickert
matthewfeickert approved these changes Dec 9, 2019
View reviewed changes
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 👍. Can we just get a PR review commit log comments? After that I'm fine with merging and then cutting a new minor release.

@lukasheinrich
Copy link
Contributor

this breaks the API, so probably should come with a versions bump

lukasheinrich
lukasheinrich requested changes Dec 9, 2019
View reviewed changes
Copy link
Contributor

@lukasheinrich lukasheinrich left a comment •
edited
Loading

Choose a reason for hiding this comment

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

hm didn't we first suggest pyhf.infer instead of pyhf.stats?

@matthewfeickert
Copy link
Member

hm didn't we first suggest pyhf.infer instead of pyhf.stats?

Ah, yes, we did. I forgot about that, but I agree that "infer" is better — it gets directly to what is being done, while "stats" is too broad.

matthewfeickert
matthewfeickert requested changes Dec 9, 2019
View reviewed changes
Copy link
Member

@matthewfeickert matthewfeickert left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We also need the following entry checked off in ROADMAP.md

(where stats has become infer).

@matthewfeickert
Copy link
Member

matthewfeickert commented Dec 9, 2019 •
edited
Loading

this breaks the API, so probably should come with a versions bump

Ah, so just doing it all in one fell swoop instead of a bump version PR? Makes sense.

@kratsg
Copy link
Contributor Author

kratsg commented Dec 9, 2019

Switched to pyhf.infer.

@kratsg kratsg changed the title refactor: Create stats subpackage to move our statistics into one location refactor: Create infer subpackage to hold our statistics code Dec 9, 2019
@kratsg kratsg force-pushed the feature/statsSubpackage branch from ba8e540 to e48f9bb Compare December 9, 2019 17:26
@kratsg kratsg added the bumpversion/minor Create a minor version release label Dec 9, 2019
matthewfeickert
matthewfeickert reviewed Dec 9, 2019
View reviewed changes
matthewfeickert
matthewfeickert requested changes Dec 9, 2019
View reviewed changes
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 don't think

'https://cdnjs.cloudflare.com/ajax/libs/github-fork-ribbon-css/0.2.2/gh-fork-ribbon.min.css'

was supposed to get changed

matthewfeickert
matthewfeickert reviewed Dec 9, 2019
View reviewed changes
matthewfeickert
matthewfeickert approved these changes Dec 9, 2019
View reviewed changes
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.

GTG again. :)

@kratsg kratsg merged commit 7b296cc into master Dec 9, 2019
@kratsg kratsg deleted the feature/statsSubpackage branch December 9, 2019 18:03
@lukasheinrich lukasheinrich mentioned this pull request Dec 16, 2019
Merged
4 tasks
@matthewfeickert matthewfeickert mentioned this pull request Feb 6, 2020
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukasheinrich lukasheinrich lukasheinrich approved these changes

@matthewfeickert matthewfeickert matthewfeickert approved these changes

Assignees

@kratsg kratsg

Labels
bumpversion/minor Create a minor version release feat/enhancement New feature or request refactor A code change that neither fixes a bug nor adds a feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kratsg @lukasheinrich @matthewfeickert