Skip to content

Comments

Add rice tests#2964

Merged
ColCarroll merged 4 commits intopymc-devs:masterfrom
MBrouns:add_rice_tests
May 19, 2018
Merged

Add rice tests#2964
ColCarroll merged 4 commits intopymc-devs:masterfrom
MBrouns:add_rice_tests

Conversation

@MBrouns
Copy link
Contributor

@MBrouns MBrouns commented May 2, 2018

Continuation from #2963. I rebased to get rid of the merge commit and got the tests to pass by changing the tested domain to Rplusbig.

The problem with large values of value and nu is that the regular i0 function will overflow to inf. Scipy seems to go around this by slightly modifying the original function from:

x * exp(-(x**2+b**2)/2) * I[0](x*b) to: x * np.exp(-(x-b)*(x-b)/2.0) * sc.i0e(x*b)

The i0e function is then partitioned into two intervals: [0, 8] and (8, infinity) which are implemented slightly different:

  if (x <= 8.0) {
	y = (x / 2.0) - 2.0;
	return (chbevl(y, A, 30));
    }

    return (chbevl(32.0 / x - 2.0, B, 25) / sqrt(x));

If we want the Rice distribution to be able to handle larger values for value and nu we would need to implement a similar strategy but I am unsure on how exactly to implement this in PyMC3. Would this PR be acceptable without that?

@MBrouns MBrouns mentioned this pull request May 2, 2018
Copy link
Member

@ColCarroll ColCarroll left a comment

Choose a reason for hiding this comment

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

Sorry this sat for so long -- looks good to me, other than the one comment!

Copy link
Member

Choose a reason for hiding this comment

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

@MBrouns
Copy link
Contributor Author

MBrouns commented May 17, 2018

Okay that was a great comment! I used the same implementation of i0 in theano to implement the i0e function which is used in scipy to improve the numerical stability of the pdf for large values of value and nu. That means the tests can use a much larger domain now for testing!

@ColCarroll
Copy link
Member

Looks like there is merging trouble. My fork is named origin, and this main version is called upstream (check with git remote -v). If your setup is the same, you would run

git fetch upstream
git rebase upstream/master
git push -u origin +add_rice_tests

Notice the + on that last line, which breaks history on your version (so don't do that on master for a general project), but won't matter to us when we merge.

twiecki and others added 3 commits May 18, 2018 19:33
…alues of nu and value

Use ((x-b)**2)/2 + xb instead of (x**2 + b**2)/2 in the pdf for the rice distribution and include the np.exp(-xb) in the i0e to match the scipy implementation
@MBrouns
Copy link
Contributor Author

MBrouns commented May 18, 2018

I can't seem to reproduce the failing test case on my local machine. I'm using python 2.7 and set the FLOATX env var to float32. Any idea what's happening?

@ColCarroll
Copy link
Member

That looks strange - like the output is -inf when you expect it to be like, -109.

Replicating the tests is a little tough (you need to set $THEANO_FLAGS, not $FLOATX). I can try on my machine later, but I think this is the invocation if you have things set up in a python 2.7 environment:

THEANO_FLAGS='floatX=float32,gcc.cxxflags='\''-march=core2'\''' python -m pytest -xv pymc3/tests/test_distributions.py::TestMatchesScipy::test_rice

@MBrouns
Copy link
Contributor Author

MBrouns commented May 19, 2018

Tests are passing now!

@ColCarroll ColCarroll merged commit cf636dc into pymc-devs:master May 19, 2018
@ColCarroll
Copy link
Member

Looks good to me -- thanks for the contribution, @MBrouns!

@MBrouns MBrouns mentioned this pull request Jul 28, 2018
@MBrouns MBrouns deleted the add_rice_tests branch December 21, 2018 08:41
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