Skip to content

Conversation

@matthewfeickert
Copy link
Member

@matthewfeickert matthewfeickert commented Oct 21, 2021

Description

Allow zero rate Poisson under the case that the limit \lambda -> 0 is well defined as

    P(n | \lambda = 0) = { 1 if n = 0
                         { 0 otherwise

This PR enforces the default behavior across all backends.

This PR effectively reverts most of PR #1001 and PR #280, and reapplies most of @wiso's PR #277.

To allow for zero rate Poisson across all backends, the mimimum required versions are adjusted:

Note, that Issue #293 introduces the idea of a cutoff. That isn't off the table, but should be discussed in a separate Issue.

(Self note: As it will probably come up again at some point, the first citation of this being a thing that I can find is from this Cross Validated post.)

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
* Effectively reverts most of PR #1001 and PR #280, reapplies most
of PR #277
* Use scipy.special.xlogy in Poisson computation for numpy backend
and use jax.scipy.special.xlogy for jax backend
* Set minimum required PyTorch to v1.10 for API stability
   - c.f. https://github.com/pytorch/pytorch/pull/61511 in torch v1.10.0
* Set minimum required TensorFlow to v2.3.1 and TensorFlow Probability
to v0.11.0
   - tfp v0.11.0 supports zero rate Poisson and requires tensorflow>=2.3.0
* Add note to docs that limit Poisson(n = 0 | lambda -> 0) = 1 is being used
* Update tests to use limit Poisson(n = 0 | lambda -> 0) = 1 result
* Run doctest on only the latest Python release

Co-authored-by: Ruggero Turra <[email protected]>

@matthewfeickert matthewfeickert added feat/enhancement New feature or request docs Documentation related tests pytest fix A bug fix labels Oct 21, 2021
@matthewfeickert matthewfeickert self-assigned this Oct 21, 2021
@matthewfeickert
Copy link
Member Author

@matthewfeickert
Copy link
Member Author

cc @wiso, @alexander-held, @andrewfowlie as you've all participated in discussion on Issue #293.

@codecov
Copy link

codecov bot commented Oct 21, 2021

Codecov Report

Merging #1657 (652813a) into master (8a3dc6b) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1657      +/-   ##
==========================================
- Coverage   98.04%   98.04%   -0.01%     
==========================================
  Files          63       63              
  Lines        4145     4142       -3     
  Branches      572      572              
==========================================
- Hits         4064     4061       -3     
  Misses         48       48              
  Partials       33       33              
Flag Coverage Δ
contrib 24.89% <25.00%> (-0.01%) ⬇️
doctest 60.98% <100.00%> (-0.03%) ⬇️
unittests 96.33% <100.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
src/pyhf/tensor/pytorch_backend.py 98.41% <ø> (ø)
src/pyhf/tensor/jax_backend.py 97.08% <100.00%> (ø)
src/pyhf/tensor/numpy_backend.py 98.47% <100.00%> (ø)
src/pyhf/tensor/tensorflow_backend.py 97.22% <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 8a3dc6b...652813a. Read the comment docs.

@matthewfeickert matthewfeickert marked this pull request as ready for review October 22, 2021 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Documentation related feat/enhancement New feature or request fix A bug fix tests pytest

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Motivation for allowing zero rate in Poisson in v0.11.0? Should Poisson[0,0] be handled especially?

3 participants