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

feat: Simultaneous pdf class #553

Merged
merged 1 commit into from
Sep 17, 2019
Merged

feat: Simultaneous pdf class #553

merged 1 commit into from
Sep 17, 2019

Conversation

lukasheinrich
Copy link
Contributor

@lukasheinrich lukasheinrich commented Sep 8, 2019 •
edited by matthewfeickert
Loading

Description

a demo PR against add_probability_module #551 showing how we will arrive at a pyfitcore like API

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
* extends pyhf.probability to have a joint pdf class pyhf.Simultaneous
* adapts pdf evaluation code to be thing layer around the pdf(pars).log_prob(data) pattern.

@lukasheinrich lukasheinrich mentioned this pull request Sep 8, 2019
Closed
3 tasks
@coveralls
Copy link

coveralls commented Sep 8, 2019 •
edited
Loading

Coverage Status

Coverage decreased (-0.4%) to 93.554% when pulling 7d4571d on add_probability_module_withjoint into d50c7e4 on master.

@lukasheinrich lukasheinrich changed the title [WIP] towards statisfactory towards statisfactory Sep 8, 2019
@lukasheinrich lukasheinrich changed the title towards statisfactory [WIP] towards statisfactory Sep 8, 2019
@lukasheinrich lukasheinrich mentioned this pull request Sep 8, 2019
Closed
@lukasheinrich lukasheinrich changed the title [WIP] towards statisfactory towards statisfactory Sep 8, 2019
@lukasheinrich lukasheinrich changed the title towards statisfactory towards pyfitcore Sep 12, 2019
@matthewfeickert
Copy link
Member

@lukasheinrich To make this easier to review (as there seems to be some overlap of file creation) can you rebase this against PR #551 (as that hasn't gotten reviewed by Giordon and merged yet)?

@lukasheinrich lukasheinrich mentioned this pull request Sep 15, 2019
Merged
4 tasks
@lukasheinrich lukasheinrich changed the base branch from add_probability_module to master September 15, 2019 21:25
@lukasheinrich lukasheinrich force-pushed the add_probability_module_withjoint branch from b6c111b to 1f66dfb Compare September 15, 2019 21:40
@matthewfeickert matthewfeickert added the feat/enhancement New feature or request label Sep 15, 2019
@lukasheinrich lukasheinrich force-pushed the add_probability_module_withjoint branch 2 times, most recently from 40b2f06 to 1bbd9c7 Compare September 15, 2019 22:19
kratsg
kratsg reviewed Sep 15, 2019
View reviewed changes
kratsg
kratsg requested changes Sep 15, 2019
View reviewed changes
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.

Why is test_validation.py changed to 1-bin from 2-bin?

matthewfeickert
matthewfeickert requested changes Sep 16, 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.

Thanks for this PR, Lukas. In addition to @kratsg's requested changes I'd like to see docstrings with examples that get run in doctest added.

Also, while there is nice functionality added in this PR it would be good to make the PR title, description, and proposed commit messages give much more explanation as to what the PRs purpose is and how it will

[show] how we will arrive at a pyfitcore like API.

@matthewfeickert matthewfeickert mentioned this pull request Sep 16, 2019
Merged
4 tasks
@lukasheinrich lukasheinrich force-pushed the add_probability_module_withjoint branch from c749944 to b328d9d Compare September 16, 2019 19:09
@matthewfeickert
Copy link
Member

The docstrings should have use examples for them. There are some instances where the example might be too complex to be helpful if the class or method isn't in the public API, but if it is in the public API we should have them.

@lukasheinrich lukasheinrich reopened this Sep 16, 2019
@lukasheinrich lukasheinrich changed the title towards pyfitcore feat: Simultaneous pdf class Sep 17, 2019
@matthewfeickert
Copy link
Member

Can we also get the "Summarize commit messages into a comprehensive review of the PR"? Once that is done I can approve this.

matthewfeickert
matthewfeickert approved these changes Sep 17, 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.

LGTM (once it is rebased to fix the CI). Thanks for the PR @lukasheinrich.

@lukasheinrich
Copy link
Contributor Author

failing now as expected. Will rebase once #564 is merged

@lukasheinrich lukasheinrich force-pushed the add_probability_module_withjoint branch from 93351bf to 7d4571d Compare September 17, 2019 21:57
@matthewfeickert matthewfeickert merged commit 86269a0 into master Sep 17, 2019
@matthewfeickert matthewfeickert deleted the add_probability_module_withjoint branch September 17, 2019 23:13
This was referenced Sep 18, 2019
Open
Merged
Merged
Closed
lukasheinrich pushed a commit that referenced this pull request Sep 19, 2019
@matthewfeickert @lukasheinrich
fix: Avoid benchmark timeout in CI systems (#571)
a033901 
* fix: Use the pytest keyword expression CLI flag -k to skip running the the 200 bin Minuit benchmark test to avoid time out errors in the CI systems due to speed regression introduced in PR #553
   - c.f. 'man pytest' and http://doc.pytest.org/en/latest/example/markers.html#using-k-expr-to-select-tests-based-on-their-name for more information on the keyword expression CLI flag
@lukasheinrich lukasheinrich mentioned this pull request Sep 30, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kratsg kratsg kratsg approved these changes

@matthewfeickert matthewfeickert matthewfeickert approved these changes

Assignees

@lukasheinrich lukasheinrich

Labels
feat/enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

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