BUG: Fixed Von Mises distribution for big values of kappa #18498
Conversation
…to a normal distribution (which the von mises distribution converges to).
…sing large values of kappa (>1e6)
82ca662 to
8e4ac94
Compare
Apply vonmises fix only to Generator Add tests for correctness closes numpy#17378 closes numpy#17275
8e4ac94 to
79c3030
Compare
| return result - 2*M_PI; | ||
| } | ||
| return result; | ||
| } else { |
There was a problem hiding this comment.
Indentation needs to be 4 spaces, else on new line.
There was a problem hiding this comment.
Style needs fixing all round, let's leave that to a later cleanup.
There was a problem hiding this comment.
While I don't mind changing the commit, the entire file is formatted using these conventions (2 spaces, single like else). IIRC think this is Google C format as formatted using clang-format.
| double rho = (r - sqrt(2 * r)) / (2 * kappa); | ||
| s = (1 + rho * rho) / (2 * rho); | ||
| #ifndef NP_RANDOM_LEGACY | ||
| /* Fallback to normal distribution for big values of kappa |
There was a problem hiding this comment.
First comment line should be blank.
|
I think that it would be safer/more readable/more convenient to do the |
|
The code coverage failure should probably be fixed as it seems a common case. |
I moved it to legacy-distributions.c as legacy_vonmises. This has the advantage of clarifying that these functions should not be altered except under special circumstances. |
| @pytest.mark.parametrize("mu", [-7., -np.pi, -3.1, np.pi, 3.2]) | ||
| @pytest.mark.parametrize("kappa", [1e-9, 1e-6, 1, 1e3, 1e15]) | ||
| def test_vonmises_large_kappa_range(self, mu, kappa): | ||
| r = random.vonmises(mu, kappa, 50) | ||
| assert_(np.all(r > -np.pi) and np.all(r <= np.pi)) | ||
|
|
There was a problem hiding this comment.
I think that coverage is a bit suspicious. I added this test to ensure that all code paths are hit.
Avoid conditional compilation and move old version to legacy_vonmises Small clean up Additional comments
8ec65ca to
aa52959
Compare
Agreed. |
|
The azure failure is unrelated, I didn't rerun the test as the output would be lost. |
|
Thanks @bashtage . |
Extends branch in #17378 to prevent fix applying to RandomState and changing its stream.