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

Rice distribution: extend to b=0, add an explicit rvs method. #2847

Merged
merged 3 commits into from
Sep 15, 2013

Conversation

ev-br
Copy link
Member

@ev-br ev-br commented Sep 9, 2013

closes gh-2164 and gh-2742.

As a bonus, rvs get a bit faster. With current master:

In [10]: scipy.__version__
Out[10]: '0.14.0.dev-efbde60'

In [11]: %timeit stats.rice.rvs(b=3.)
100 loops, best of 3: 4.18 ms per loop

In [12]: %timeit stats.rice.rvs(b=3., size=1000)
1 loops, best of 3: 4.13 s per loop

with this PR:

In [6]: scipy.__version__
Out[6]: '0.14.0.dev-bf7d8d9'

In [7]: %timeit stats.rice.rvs(b=3.)
10000 loops, best of 3: 45.5 µs per loop

In [8]: %timeit stats.rice.rvs(b=3., size=1000)
10000 loops, best of 3: 192 µs per loop

@josef-pkt
Copy link
Member

nice, ticket count is going down

looks good to me

# rice.pdf(x, b\to 0) = x exp(-x^2/2) + O(b^2)
# see e.g. Abramovich & Stegun 9.6.7 & 9.6.10
b = 1e-8
np.allclose(stats.rice.pdf(x, 0), stats.rice.pdf(x, b),
Copy link
Member

Choose a reason for hiding this comment

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

I think you meant to use assert_allclose here?

@rgommers
Copy link
Member

Looks good. One thing to add would be to remove 'rice' from the list of slow distributions in test_continuous_basic.py

@ev-br
Copy link
Member Author

ev-br commented Sep 15, 2013

made changed suggested by @rgommers, rebased on master, pushed the rebased version.

@rgommers
Copy link
Member

Slight git mishap here....

@ev-br
Copy link
Member Author

ev-br commented Sep 15, 2013

ouch.
I screwed it up.
any suggestion of how to fix this mess? Or is it easier to close this, apply the changes to the master, and send a fresh PR?

@rgommers
Copy link
Member

You can create a new branch from current master, cherry-pick the relevant commits and then force push to this PR ($ git push origin newbranch:rice_enh).

@ev-br
Copy link
Member Author

ev-br commented Sep 15, 2013

@rgommers done just that, thanks a whole lot!
Just for me to understand (and avoid in the future): what was the error?
What I did was

  • fetch upstream master and merge it to the local master
  • rebased the feature branch on local master
  • tried pushing to the origin, which failed
  • pulled the feature branch from the origin, resolved merge conflicts,
  • pushed the feature branch back to origin
    As there was no force-pushing involved, I thought it'd to the right thing.
    Apparently not...

@josef-pkt
Copy link
Member

after rebase you need to force push to your github branch, to overwrite it

@rgommers
Copy link
Member

Yep. Step 1, 2 are correct, 3 needed -f.

rgommers added a commit that referenced this pull request Sep 15, 2013
Rice distribution: extend to b=0, add an explicit rvs method.
@rgommers rgommers merged commit f1b8578 into scipy:master Sep 15, 2013
@rgommers
Copy link
Member

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stats.rice.pdf(x, 0) returns nan (Trac #1639)
3 participants