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

Removed Laplace from the list of fit.md #1187

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Dsantra92
Copy link
Contributor

As requested in #807, I have removed the Laplace from the list of distribution from the list in fit.md .

@Dsantra92
Copy link
Contributor Author

On another note Laplace supports fit_mle(dist, x) and does not support fit_mle(dist, x, w). I would like to ask if I could add the fit_mle(dist,x,w) for Laplace in a future PR.

@andreasnoack
Copy link
Member

I don't think removing it from the list is the way to go here. As you mention, fit_mle(dist, x) works so the right thing seems to be to clarify which of the distributions support the weight option in fit_mle

@Dsantra92
Copy link
Contributor Author

@andreasnoack Can I just make a PR to add weighted_fit_mle for Laplacian ? I guess that will resolve the issue.

@andreasnoack
Copy link
Member

Yes. That would be even better.

@Dsantra92
Copy link
Contributor Author

Dsantra92 commented Oct 16, 2020

Laplace weighted fit_mle requires weighted MAD and I found no implementation in StatsBase.jl. I implemented my own MAD and realized it's too messy for a single function to holdv(which is not even intended to it). I actually see 3 options:

  1. Implement weighted MAD in StatsBase.jl and then use it.
  2. Implement weighted map in the fit_mle function itself.
  3. Or as you suggested, just mention that the weighted fit_mle is not implemented in the package.

I believe 3 would be a quick fix for now and I will add the fit_mle with MAD in a follow-up pr.

@codecov-commenter
Copy link

codecov-commenter commented May 8, 2021

Codecov Report

Merging #1187 (c4e4f3a) into master (e127e9d) will decrease coverage by 0.38%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1187      +/-   ##
==========================================
- Coverage   81.89%   81.51%   -0.39%     
==========================================
  Files         116      115       -1     
  Lines        6534     6626      +92     
==========================================
+ Hits         5351     5401      +50     
- Misses       1183     1225      +42     
Impacted Files Coverage Δ
src/matrixvariates.jl 76.81% <0.00%> (-14.10%) ⬇️
src/multivariates.jl 70.66% <0.00%> (-10.92%) ⬇️
src/mixtures/mixturemodel.jl 68.11% <0.00%> (-10.49%) ⬇️
src/univariates.jl 62.20% <0.00%> (-5.35%) ⬇️
src/univariate/continuous/logitnormal.jl 64.28% <0.00%> (-4.95%) ⬇️
src/univariate/discrete/hypergeometric.jl 63.41% <0.00%> (-3.26%) ⬇️
src/univariate/discrete/skellam.jl 81.25% <0.00%> (-2.63%) ⬇️
src/univariate/continuous/pgeneralizedgaussian.jl 61.76% <0.00%> (-1.88%) ⬇️
src/univariate/discrete/soliton.jl 90.90% <0.00%> (-1.69%) ⬇️
src/common.jl 67.56% <0.00%> (-1.67%) ⬇️
... and 41 more

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 e127e9d...c4e4f3a. Read the comment docs.

docs/src/fit.md Outdated Show resolved Hide resolved
@devmotion
Copy link
Member

IIRC this was fixed recently.

@devmotion
Copy link
Member

Ah no, the unweighted fit was fixed (#1309) and a PR for the weighted fit was closed (#1310) but is planned to be updated (possibly in a new PR) after JuliaStats/StatsBase.jl#687 is fixed. So depending on how timely the weighted fit is implemented, the warning in this PR might not be needed.

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.

None yet

5 participants