[ENH] zero-inflated distribution#648
Conversation
f6ac48c to
7a3d255
Compare
fkiraly
left a comment
There was a problem hiding this comment.
very nice!
Could we please move the modifications to TruncatedDistribution to another PR, since I think there is some discussion to be had about how to parameterize the options, e.g., why only lower inclusive and not upper inclusive, etc.
The zero inflated distribution looks good.
|
This can now be re-reviewed as #667 is merged |
|
@fkiraly, just bringing this to your attention this needs a review. Thanks! |
fkiraly
left a comment
There was a problem hiding this comment.
Very nice!
Two main comments above:
- make
puse the broadcast logic built into the base class, instead of replacing that with a different logic. That way it is consistent. - I would also avoid truncating, actually. I would treat the distribution as a special mixture, instead, this also reduces hard to track case distinctions.
|
Thanks for the review, and apologies for the delayed reply. I can work on addressing the comments however as #713 works on the same issue., should I continue working on this PR? |
|
@Khushmagrawal, yes, please continue. I think there are useful things that both PR have that the other does not have. In either case you would be credited as a contributor. |
|
@Khushmagrawal - Are you still actively working on this? If so, I'm ok with merging the generic ZI distribution work. I can focus on the bespoke ZI distributions for the most used distributions. |
Ah yes, I’m still working on this. I was waiting to reach a conclusion on the truncation discussion above. For now, I have added a commit removing the truncation part, if the current code looks good then we can proceed with merging |
|
@Khushmagrawal - By "merging" in me previous message I meant to say combine, as I've also worked a bit on this myself. I'll try to get around to compare/review it soon. |
Reference Issues/PRs
Partially towards #554
Blocked by #667What does this implement/fix? Explain your changes.
Adds support for Zero-inflated distribution
Does your contribution introduce a new dependency? If yes, which one?
No.
What should a reviewer concentrate their feedback on?
Design decision of making the lower bound inclusive intruncated.pyDid you add any tests for the change?
No
Any other comments?
After the ZeroInflated implementation is reviewed, I plan to add
ZeroInflatedPoissonandZeroInflatedNegativeBinomialas well using_DelegatedDistributionPR checklist
For all contributions
How to: add yourself to the all-contributors file in the
skproroot directory (not theCONTRIBUTORS.md). Common badges:code- fixing a bug, or adding code logic.doc- writing or improving documentation or docstrings.bug- reporting or diagnosing a bug (get this pluscodeif you also fixed the bug in the PR).maintenance- CI, test framework, release.See here for full badge reference
For new estimators
docs/source/api_reference/taskname.rst, follow the pattern.Examplessection.python_dependenciestag and ensureddependency isolation, see the estimator dependencies guide.