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

Added two wrappers to the gaussian_kde class. #3398

Closed
wants to merge 4 commits into from
Closed

Added two wrappers to the gaussian_kde class. #3398

wants to merge 4 commits into from

Conversation

nicodelpiano
Copy link
Contributor

The wrappers are:

  • pdf
  • logpdf

The wrappers are:
- pdf
- logpdf
@rgommers
Copy link
Member

These are new public methods, so should get docstrings.

Adding unit tests is also necessary. Those live in ``scipy/stats/tests/test_kdeoth.py. You can look at existing tests and add one for each new method.

@rgommers
Copy link
Member

@nicodelpiano are you planning to finish this?

@nicodelpiano
Copy link
Contributor Author

Yes, I was too busy with my master, sorry for that.
About the documentation, is it okay to put "Computes the probability density function" for the pdf wrapper (and something similar for logpdf)?

@rgommers
Copy link
Member

Probably for pdf the best thing to do is copy the first line of the evaluate docstring (it is a copy after all) and then add one sentence explaining that those two are equivalent and see the evaluate docstring for more details.

And for logpdf something similar.

@nicodelpiano
Copy link
Contributor Author

I am thinking that the unit test for pdf will be the same as the "test_kde_bandwidth_method" but changing the "evaluate" calls for "pdf".
Do you think this is correct?

@josef-pkt
Copy link
Member

For the unit tests, I think the intention of the test would be more obvious if you just compare pdf and evaluate on an example.

for the docstring: does it work to just assign the evaluate docstring,
pdf.__doc__ = evaluate.__doc__ ?

@nicodelpiano
Copy link
Contributor Author

What do you think of this?

def test_pdf():
    n_basesample = 50
    xn = np.random.randn(n_basesample)

    gkde = stats.gaussian_kde(xn)

    xs = np.linspace(-15,12,25)
    pdf = gkde.evaluate(xs)
    pdf2 = gkde.pdf(xs)
    assert_almost_equal(pdf, pdf2)

@josef-pkt
Copy link
Member

Yes I think it's good, it's directly to the point of checking an alias.
Two points:

  • add a np.random.seed(1) so we get deterministic repeatable results
  • reduce the tolerance in the assert, once you have a seed you can look at how close the values are. They won't be exactly the same because of numerical imprecision, but we could have for example decimal=12, or set atol and rtol for assert_allclose.

@nicodelpiano
Copy link
Contributor Author

How about this?

def test_pdf():
    np.random.seed(1)
    n_basesample = 50
    xn = np.random.randn(n_basesample)

    # Default
    gkde = stats.gaussian_kde(xn)

    xs = np.linspace(-15,12,25)
    pdf = gkde.evaluate(xs)
    pdf2 = gkde.pdf(xs)
    assert_almost_equal(pdf, pdf2, decimal=12)

@josef-pkt
Copy link
Member

looks good to me

I think you can also add the assert_almost_equal(gkde.logpdf(xs), np.log(pdf), ...) to this test function.

The wrappers are:
- pdf
- logpdf
The wrappers are:
- pdf
- logpdf
The wrappers are:
- pdf
- logpdf
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 161f1a6 on nicodelpiano:master into * on scipy:master*.

@nicodelpiano
Copy link
Contributor Author

Is there something wrong in the patch? please let me know!

@josef-pkt
Copy link
Member

looks good to me

(I would still prefer full docstrings)

@rgommers
Copy link
Member

@nicodelpiano indeed nothing wrong with this patch. Can be merged, with maybe a tweak to the docstrings.

rgommers added a commit that referenced this pull request May 10, 2014
@rgommers
Copy link
Member

Merged with some changes in c0864c9. Thanks @nicodelpiano

@rgommers rgommers closed this May 10, 2014
@rgommers rgommers added this to the 0.15.0 milestone May 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement scipy.stats
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants