-
Notifications
You must be signed in to change notification settings - Fork 45
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
[ENH] Pareto distribution #396
Conversation
skpro/distributions/pareto.py
Outdated
|
||
Parameters | ||
---------- | ||
xm : float or array of float (1D or 2D), must be positive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
xm is as it is on wikipedia, but I think it is confusing since there is another parameter x, below. Shall we name it scale
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that would be nice. I just used the Wikipedia convention initially as different websites had different names for these two parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have implemented the parameter name change from xm to scale, and updated the docstring as such
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have added the CDF to the docstring for clarity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. May I request a parameter rename, as above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
Not blocking, but it would be great if you could add explicitly cdf or pdf in the docstring, so it is clear which parameterization this class is using.
You can see examples for this in other classes.
Reference Issues/PRs
Addresses #22 for the Pareto distribution
What does this implement/fix? Explain your changes.
Implements some methods for the Pareto distribution
Does your contribution introduce a new dependency? If yes, which one?
No
What should a reviewer concentrate their feedback on?
Current state of code, and how to add energy
Did you add any tests for the change?
No
Any other comments?
No
PR checklist
For all contributions
How to: add yourself to the all-contributors file in the
skpro
root 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 pluscode
if 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.Examples
section.python_dependencies
tag and ensureddependency isolation, see the estimator dependencies guide.