[ENH] exact _log_pdf for Uniform distribution#784
Conversation
|
Hey @fkiraly @felipeangelimvieira |
|
Hi @Ashish-Kumar-Dash, I recommend working on further issues than waiting for this. Also the reason I found was that the change was better explained and written in a way that follows skpro more closely than yours. Rather than keeping this pr active and increasing review load on reviewer, I would close this pr and look for issues under |
|
Hey @arnavk23 , |
_log_pdf for Uniform distribution
|
I accidentally reviewed #787 which is a copy of this. Instead, this PR should be merged. |
|
FYI, @arnavk23 is not a maintainer. Please ignore what he said. |
fkiraly
left a comment
There was a problem hiding this comment.
Sorry for the mixup. Thanks for the contribution, and welcome to sktime / skpro!
|
thanks for the acknowledgement and prompt response! |
|
I also apologise for the mixup on this issue and pr. I think I was a bit premature in my understanding of the changes made in the two prs. |
Reference Issues/PRs
Fixes #782
What does this implement/fix? Explain your changes.
Adds the missing
_log_pdfmethod to theUniformdistribution. The class already declared"log_pdf"in itscapabilities:exacttag but never implemented the method, causing the base class fallback (np.log(self.pdf(x))) to be used at runtime. This fallback is numerically unstable for out-of-bounds points (log(0)triggers numpy warnings) and contradicts the exact tag.The new implementation directly computes:
-np.log(upper - lower)for in-bounds points-np.inffor out-of-bounds pointsusing
np.where, which is both exact and numerically clean.Does your contribution introduce a new dependency? If yes, which one?
No.
What should a reviewer concentrate their feedback on?
The
_log_pdfimplementation in uniform.py — single method addition following the same pattern asLaplace._log_pdfandNormal._log_pdf.Did you add any tests for the change?
No new tests were added. The existing
test_log_pdf_and_pdftest intest_all_distrs.pyalready validates consistency betweenpdfandlog_pdffor all distributions, includingUniform. All 97 existing Uniform tests pass.Any other comments?
This is a small, low-risk change — one method addition (~18 lines including docstring) with no changes to existing logic.
PR checklist
For all contributions
[ENH],[MNT],[DOC], or[BUG].[ENH]- adding or improving code.