-
Notifications
You must be signed in to change notification settings - Fork 74
[ENH] Add BurrIII, BurrXII, FDist, FatigueLife, GeneralizedPareto, Levy, Skellam, TruncatedPareto distributions #690
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
base: main
Are you sure you want to change the base?
Conversation
…y, Skellam, TruncatedPareto distributions and update docs
… parameters are public attributes and variance returns positive values for test compatibility. All tests pass.
…GeneralizedPareto to resolve pydocstyle D102 errors.
fkiraly
left a comment
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 - looks AI generated, and incomplete.
If you use AI, you need to check for mistakes and hallucinations.
- please complete docstrings with an explanation of the distribution, e.g., by giving its definition through
pdforcdf. - some distributions simply return
np.nanthrough_mean, etc. This overrides the ppf-quadrature based default, so it is not useful. - there is already a standing scipy adapter which you can use, instead of copy-pasting multiple times the calls to
scipy. Please refer to existing distributions that use thescipybackend.
caf0f90 to
15e4d0d
Compare
This pr was not generated by any LLM. Point 1 was a welcome addition (I had done the calculation but forgot to add) Point 2 also another welcome addition but I didn't know about Point 3 (Thank you)
|
|
@fkiraly |
5f14139 to
15983fe
Compare
Ok, I see - it is fine to use AI (I also do), but you need to check what it does. It does not make sense to return a dataframe full of nans, if the default already implements a quadrature mean. |
I agree, that is why I was working on it (which I should've put in a draft pr). Review this pr with a bit of more scrutiny still. Many files are here so mistakes can still be hidden somewhere but I have tried my best. |
skpro/distributions/adapters/scipy/tests/test_scipy_adapters.py
Outdated
Show resolved
Hide resolved
fkiraly
left a comment
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 think this is still not good, and AI generated content / hallucinations remain.
- the
_ScipyAdapteris not properly used. Have a look at how, say,Gammalooks like. - there are still instances of AI problems in the code. If you use AI, please ensure you go through and check.
- the API test contract should not be changed with stepouts for individual distributions. Instead, you can skip an individual test, but please comment on what does not work. Or, if you think the API contract should change, please explain.
The only remaining test failure is for |
|
@fkiraly If you like, we could also just take out burriii from this pr if all the other files are ok to you and do the implementation in a followup pr if it keeps on causing problems |
Reference Issues/PRs
Towards #22
What does this implement/fix? Explain your changes.
Does your contribution introduce a new dependency? If yes, which one?
What should a reviewer concentrate their feedback on?
Did you add any tests for the change?
Any other comments?
PR 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.