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

Add probability module #551

Merged
merged 28 commits into from
Sep 15, 2019
Merged

Add probability module #551

merged 28 commits into from
Sep 15, 2019

Conversation

lukasheinrich
Copy link
Contributor

@lukasheinrich lukasheinrich commented Sep 6, 2019

Description

another step towards statisfactory. this replaces log_prob(data,pars) calls directly to tensorlib with objects that mirror the API of torch.distributions.Distribution

see e.g. test (which should probably be added here)

https://gist.github.com/lukasheinrich/c69d89333d1c5bd6ea6a0a78eacc919b

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
* adds pyhf.probability module
* pyhf.probability : stats classes of backends  :: tensorlib : array classes of backends

@lukasheinrich lukasheinrich changed the title add probability module. [WIP] add probability module. Sep 6, 2019
@coveralls
Copy link

coveralls commented Sep 6, 2019

Coverage Status

Coverage increased (+0.06%) to 93.819% when pulling 359e7ed on add_probability_module into 57131f7 on master.

@lukasheinrich lukasheinrich changed the title [WIP] add probability module. Add probability module. Sep 6, 2019
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.

It would be nice to start getting docstrings on both the classes and their methods.

@matthewfeickert matthewfeickert changed the title Add probability module. Add probability module Sep 7, 2019
@lukasheinrich lukasheinrich mentioned this pull request Sep 8, 2019
4 tasks
@matthewfeickert matthewfeickert added API Changes the public API feat/enhancement New feature or request labels Sep 9, 2019
@matthewfeickert matthewfeickert mentioned this pull request Sep 9, 2019
4 tasks
@matthewfeickert
Copy link
Member

I removed the docstrings I added and migrated them to PR #558

@matthewfeickert
Copy link
Member

We also need a summary commit message for the PR

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.

We should have some explicit tests of the probability API. I know that this will need to change a bit as the other PRs happen, but I think that it is important to make sure that we are explicitly testing each PR.

@matthewfeickert matthewfeickert mentioned this pull request Sep 10, 2019
41 tasks
lukasheinrich and others added 3 commits September 10, 2019 09:13
Co-Authored-By: Matthew Feickert <matthew.feickert@cern.ch>
@lukasheinrich
Copy link
Contributor Author

agreed. tests added

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.

@lukasheinrich Thanks for your patience and for adding tests for probability! LGreatTM!

The only thing left that I will ask is that you

Summarize commit messages into a comprehensive review of the PR

but beyond that I'm happy so I'll approve and leave this to you and @kratsg.

Copy link
Contributor

@kratsg kratsg left a comment

Choose a reason for hiding this comment

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

I would just like some clarification about why we do log_prob for the name. This doesn't seem obvious to me.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants