added icdf for asymmetric laplace distribution #7141
Conversation
|
@larryshamalama please take a look at my pull request |
|
Are there any changes needed to be done ? |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #7141 +/- ##
===========================================
- Coverage 92.21% 71.34% -20.87%
===========================================
Files 101 101
Lines 16912 16967 +55
===========================================
- Hits 15595 12105 -3490
- Misses 1317 4862 +3545
|
There was a problem hiding this comment.
Hi @ParamThakkar123, thanks for the PR. Two main comments:
- A unit test is required for any addition. You can see any merged PR from Add icdf functions for distributions #6612, e.g. Add icdf function for Cauchy and Logistic distributions #6747 or Added ICDF for the exponential distribution #6641. This unit test will automatically check if the math is correct
- You need to use
check_icdf_valueandcheck_icdf_parametersto ensure that inputs/outputs are valid (see linked PRs in previous comment).
pymc/distributions/continuous.py
Outdated
| @@ -1642,6 +1642,15 @@ def logp(value, b, kappa, mu): | |||
| msg="b > 0, kappa > 0", | |||
| ) | |||
|
|
|||
| def icdf(p, mu, b, kappa): | |||
| if b <= 0: | |||
| raise ValueError("Scale Parameter sigma must be positive") | |||
There was a problem hiding this comment.
Oh, sorry. Actually the resource from where I was referring the icdf equation, there they used sigma instead of b. I wrote it out of mistake. I will correct that one and change it to b
pymc/distributions/continuous.py
Outdated
| @@ -1642,6 +1642,15 @@ def logp(value, b, kappa, mu): | |||
| msg="b > 0, kappa > 0", | |||
| ) | |||
|
|
|||
| def icdf(p, mu, b, kappa): | |||
There was a problem hiding this comment.
| def icdf(p, mu, b, kappa): | |
| def icdf(value, b, kappa, mu): |
To keep consistent with the signatures of other icdf methods and the signatures of methods under this class
|
@larryshamalama I made the changes. |
|
Have you checked if the test passes locally? That would be an indication, good or bad, if you are on the right track |
pymc/distributions/continuous.py
Outdated
| raise ValueError("Scale Parameter b must be positive") | ||
| if value <= 0.5: | ||
| res = mu - (b / kappa) * pt.log(2 * value) | ||
| if value > 0.5: |
There was a problem hiding this comment.
Ah, comparisons need to be done using PyTensor operations
Again, there may be other edits that need to be incorporated, but a robust way to go about this is to check via a unit test, hence the emphasis for testing
There was a problem hiding this comment.
@larryshamalama I changed the comparisons to Pytensor Operations
|
@ricardoV94 I changed the if else conditions in python to pytensor condition |
| pt.le(value, 0.5), | ||
| lambda: mu - (b / kappa) * pt.log(2 * value), | ||
| lambda: mu + (b / kappa) * pt.log(2 * (1 - value)) | ||
| ) |
There was a problem hiding this comment.
Why lambda functions here...?
| pm.AsymmetricLaplace, | ||
| {"b": Rplus, "kappa": Rplus, "mu": R}, | ||
| lambda q, kappa, mu, b: st.laplace_asymmetric.ppf(q, kappa, mu, b) | ||
| ) |
There was a problem hiding this comment.
Have you checked if this test pass?
Description
Added icdf function for Asymmetric Laplace distribution
Related Issue
Checklist
Type of change
📚 Documentation preview 📚: https://pymc--7141.org.readthedocs.build/en/7141/