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

Add asymetric laplace distribution #4351

Closed
twiecki opened this issue Dec 18, 2020 · 15 comments
Closed

Add asymetric laplace distribution #4351

twiecki opened this issue Dec 18, 2020 · 15 comments

Comments

@twiecki
Copy link
Member

twiecki commented Dec 18, 2020

There is an implementation here we could port quite easily: https://github.com/epidemics/COVIDNPIs/blob/manuscript/epimodel/pymc3_distributions/asymmetric_laplace.py

@twiecki twiecki changed the title Add assymetric laplace distribution Add asymetric laplace distribution Dec 18, 2020
@chandan5362
Copy link
Contributor

Hey @twiecki , could you please explain the issue a bit more. I would love to work on this issue.

@twiecki
Copy link
Member Author

twiecki commented Dec 19, 2020

@chandan5362 That's great. As you can see in the link, the distribution is already implemented. So you would copy & paste that into distributions/continuous.py, make sure the doc string matches the ones we already have (e.g. creates a plot) and add unittests.

@chandan5362
Copy link
Contributor

@twiecki Thanks for the help. I am little confused here. where should I add unittests ?. whether should I create separate file or I will have to add it to the existing one.

@ricardoV94
Copy link
Member

@chandan5362 you should add your unit test in the existing files with unit tests for the other distributions:
https://www.github.com/pymc-devs/pymc3/tree/master/pymc3%2Ftests%2Ftest_distributions.py

You can also have a look and check what kind of tests are there for the existing distributions.

@CloudChaoszero
Copy link
Contributor

CloudChaoszero commented Dec 24, 2020

If there are no assignees to this sometime soon, I am happy to submit a PR in the near future.

  • Feel free to @ me.

@twiecki
Copy link
Member Author

twiecki commented Dec 24, 2020

@chandan5362 Are you actively working on this?
@CloudChaoszero I would say you can start tomorrow if you we don't hear.

@chandan5362
Copy link
Contributor

@twiecki @CloudChaoszero yeah, I am still working on it and I will make a PR tomorrow or day after tomorrow.

@CloudChaoszero
Copy link
Contributor

@twiecki @CloudChaoszero yeah, I am still working on it and I will make a PR tomorrow or day after tomorrow.

Oh hey, no worries @chandan5362, just checking to see if it was officially active. haha
(Take your time, it's the holidays 😄)

@chandan5362
Copy link
Contributor

hey @twiecki , as https://github.com/epidemics/COVIDNPIs/blob/manuscript/epimodel/pymc3_distributions/asymmetric_laplace.py contains only logpdf and all other instances of test functions in test_distribution.py contains both logpdf and logcdf. So, would it be alright if I add unit test only for logpdf. Since I am new to this repo and still understanding the codebase, a little help in writing the unit test would be much helpful 😐.
Thanks in advance.

@twiecki
Copy link
Member Author

twiecki commented Dec 27, 2020

@chandan5362 It's fine to not have a logcdf for this one. You can just copy a test for another distribution in test_distribution.py where we compare against the scipy implementation, and use https://scipy.github.io/devdocs/generated/scipy.stats.laplace_asymmetric.html. Does that make sense?

@chandan5362
Copy link
Contributor

yeah , sounds cool.
Thanks a lot @twiecki

@chandan5362
Copy link
Contributor

hey @twiecki , would you please help me here a little ?
As i can see that the current version of SciPy that we are using is 1.5.4 and that does not support laplace_assymetric. and the latest version of SciPy is not released yet. So, I am not able to import it for the unit test. How should I deal with it ?

@chandan5362
Copy link
Contributor

sounds cool.
thanks @ricardoV94

@Sayam753
Copy link
Member

Sayam753 commented Jan 5, 2021

Closed in #4392

@Sayam753 Sayam753 closed this as completed Jan 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants