Skip to content

Conversation

@arnavk23
Copy link
Contributor

@arnavk23 arnavk23 commented Dec 23, 2025

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
  • I've added myself to the list of contributors with any new badges I've earned :-)
    How to: add yourself to the all-contributors file in the skpro root directory (not the CONTRIBUTORS.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 plus code if you also fixed the bug in the PR).maintenance - CI, test framework, release.
    See here for full badge reference
  • The PR title starts with either [ENH], [MNT], [DOC], or [BUG]. [BUG] - bugfix, [MNT] - CI, test framework, [ENH] - adding or improving code, [DOC] - writing or improving documentation or docstrings.
For new estimators
  • I've added the estimator to the API reference - in docs/source/api_reference/taskname.rst, follow the pattern.
  • I've added one or more illustrative usage examples to the docstring, in a pydocstyle compliant Examples section.
  • If the estimator relies on a soft dependency, I've set the python_dependencies tag and ensured
    dependency isolation, see the estimator dependencies guide.

@fkiraly fkiraly added enhancement module:probability&simulation probability distributions and simulators labels Dec 23, 2025
Copy link
Collaborator

@fkiraly fkiraly left a 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 pdf or cdf.
  • some distributions simply return np.nan through _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 the scipy backend.

@arnavk23 arnavk23 marked this pull request as draft December 24, 2025 12:15
@arnavk23 arnavk23 force-pushed the fix/issue-22-distributions-main branch from caf0f90 to 15e4d0d Compare December 24, 2025 12:33
@arnavk23
Copy link
Contributor Author

  • some distributions simply return np.nan through _mean, etc. This overrides the ppf-quadrature based default, so it is not useful.

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)

  • Docstrings for BurrIII, BurrXII, FDist, FatigueLife, Skellam, and TruncatedPareto have been completed with mathematical definitions and explanations of their distributions. If you need further improvements or want similar updates for other files, let me know!
  • The overrides for _mean and _var in TruncatedPareto have been removed. The class will now use the default quadrature-based computation for these methods, as intended.
  • All new distributions (BurrIII, BurrXII, FDist, FatigueLife, Skellam) have been refactored to use the ScipyAdapter for consistency and code reuse. TruncatedPareto is a custom case and not a direct scipy distribution, so it was not adapted.

@arnavk23
Copy link
Contributor Author

@fkiraly
new_distributions_calculations.pdf
Please see the calculations below.

@arnavk23 arnavk23 force-pushed the fix/issue-22-distributions-main branch from 5f14139 to 15983fe Compare December 24, 2025 14:04
@arnavk23 arnavk23 marked this pull request as ready for review December 24, 2025 14:25
@arnavk23 arnavk23 requested a review from fkiraly December 24, 2025 14:25
@fkiraly
Copy link
Collaborator

fkiraly commented Dec 24, 2025

This pr was not generated by any LLM.

Ok, I see - it is fine to use AI (I also do), but you need to check what it does.
Empty functions or changes in unrelated files are very typical, the functions that returned np.nan were why I thought this was AI.

It does not make sense to return a dataframe full of nans, if the default already implements a quadrature mean.

@arnavk23
Copy link
Contributor Author

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.

Copy link
Collaborator

@fkiraly fkiraly left a 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 _ScipyAdapter is not properly used. Have a look at how, say, Gamma looks 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.

@arnavk23
Copy link
Contributor Author

* 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 BurrIII(c=2.0) variance, where your code returns np.inf and SciPy returns np.nan, so np.allclose(inf, nan) is False. For c <= 2, SciPy returns np.nan for the variance, but the skpro test contract expects a real non-negative value (not nan). Returning np.nan here matches SciPy's behavior and passes the adapter test, but will fail the contract test.

@arnavk23
Copy link
Contributor Author

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement module:probability&simulation probability distributions and simulators

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants