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 fit for Laplace distribution #1309

Merged
merged 6 commits into from
Apr 21, 2021
Merged

fix fit for Laplace distribution #1309

merged 6 commits into from
Apr 21, 2021

Conversation

salbert83
Copy link
Contributor

The current mle fit for Laplace calculates the mean using the median absolute deviation instead of mean absolute deviation. The former is not MLE. Also, I added functionality for a weighted Laplace MLE.

@codecov-commenter
Copy link

codecov-commenter commented Apr 21, 2021

Codecov Report

Merging #1309 (857debf) into master (ac372f0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 857debf differs from pull request most recent head e3c70c0. Consider uploading reports for the commit e3c70c0 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1309   +/-   ##
=======================================
  Coverage   81.57%   81.58%           
=======================================
  Files         115      115           
  Lines        6649     6651    +2     
=======================================
+ Hits         5424     5426    +2     
  Misses       1225     1225           
Impacted Files Coverage Δ
src/univariate/continuous/laplace.jl 92.15% <100.00%> (+0.32%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac372f0...e3c70c0. Read the comment docs.

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

Great, thanks for condensing the PR! One final question: could you check if it is possible to reduce the tolerance in

@test isapprox(scale(d) , 3.0, atol=0.2)
? I am curious if it is possible to improve the tests which apparently missed this bug.

@salbert83
Copy link
Contributor Author

Yes, I think reducing the tolerance should catch it. As a minor note, the MLE of the mean for Laplace need not be unique for an even number of samples. I know median does select a unique value. I would use "N+1" in place of "N" for the number of generated values. Then, since there are closed forms for both the location and scale parameters for this distribution and the random seed is fixed, I think a much smaller tolerance would do.

I actually came across this when testing my implementation of the weighted MLE code, which I will put in a separate pull request. Thank you!

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

Thanks again, I updated the tests as well.

@devmotion devmotion changed the title added weighted fit for Laplace distribution fix fit for Laplace distribution Apr 21, 2021
@devmotion devmotion merged commit 2b00d94 into JuliaStats:master Apr 21, 2021
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