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

Fix: bug in benchmark tests #280

Conversation

kratsg
Copy link
Contributor

@kratsg kratsg commented Sep 20, 2018

Description

The benchmark tests were not running because of forgetting to pip install minuit dependency. This was introduced through #276 which imports the minuit optimizer in tests/conftest.py which raises the exception.

This also reverts the unfortunate regression introduced by #277 as we don't have equivalent methods to return 0 for the other backends, and whether the poisson for lambda=0 should return 0 or NaN is probably not well-defined? (correct me if I'm wrong).

Checklist Before Requesting Approver

  • Tests are passing
  • "WIP" removed from the title of the pull request

@matthewfeickert
Copy link
Member

Thanks for getting this, @kratsg.

whether the poisson for lambda=0 should return 0 or NaN is probably not well-defined

Now that I think about it NaN is really the correct behavior as the Poisson p.m.f. is only defined for lambda>0.

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

@kratsg kratsg changed the base branch from master to feature/use-distributions-in-ML-backends-for-better-poisson September 20, 2018 15:22
@coveralls
Copy link

coveralls commented Sep 20, 2018

Coverage Status

Coverage remained the same at 49.322% when pulling fafe676 on bug/benchmarkTests into 94b6e8e on feature/use-distributions-in-ML-backends-for-better-poisson.

@matthewfeickert matthewfeickert force-pushed the feature/use-distributions-in-ML-backends-for-better-poisson branch from 975805b to 94b6e8e Compare September 20, 2018 15:34
@kratsg
Copy link
Contributor Author

kratsg commented Sep 20, 2018

rebased against #268 to prepare thrusters for merge.

@matthewfeickert matthewfeickert merged commit fafe676 into feature/use-distributions-in-ML-backends-for-better-poisson Sep 20, 2018
@kratsg kratsg deleted the bug/benchmarkTests branch September 20, 2018 15:57
lukasheinrich pushed a commit that referenced this pull request Oct 22, 2021
* 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. pytorch/pytorch#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 <giurrero@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants