Skip to content

Add DistfitFitter distribution fitter wrapping the distfit package - #1097

Open
pyarchana wants to merge 7 commits into
sktime:mainfrom
pyarchana:1086-distfit-fitter
Open

Add DistfitFitter distribution fitter wrapping the distfit package#1097
pyarchana wants to merge 7 commits into
sktime:mainfrom
pyarchana:1086-distfit-fitter

Conversation

@pyarchana

Copy link
Copy Markdown

Reference Issues/PRs

Towards #1086.

What does this implement/fix? Explain your changes.

Adds DistfitFitter, a skpro distribution fitter that wraps the distfit package's parametric fitting mode: it fits a set of candidate scipy.stats distributions to the data, scores them, and returns the best-scoring one as a fitted skpro scalar distribution. It follows the existing EmpiricalFitter/NormalFitter pattern (_fit/_proba, get_test_params).

Rather than reimplementing pdf/cdf/mean/var per distribution, the returned distribution reuses skpro's existing _ScipyAdapter (skpro/distributions/adapters/scipy/) via a small private wrapper (_DistfitDistribution) that takes the scipy distribution name and shape/loc/scale that distfit produces.

Currently scoped to distfit's method="parametric" mode only. I checked distfit's other three modes (quantile, percentile, discrete) and they return a differently shaped model dict that this adapter doesn't handle yet. Happy to extend if that's wanted before merge, or leave it for a follow-up.

Does your contribution introduce a new dependency? If yes, which one?

distfit, but it's already listed in pyproject.toml's all_extras, so no dependency-list changes were needed.

What should a reviewer concentrate their feedback on?

Did you add any tests for the change?

Yes: skpro/distfitter/tests/test_distfitfitter.py, plus it's automatically picked up by the existing generic TestAllDistFitters contract test suite.

Any other comments?

Opened as draft for early feedback on scope. Full local test suite, doctest, and lint (black/isort/flake8/pydocstyle) pass.

Note: docs/source/api_reference/distfitter.rst doesn't exist yet. The whole distfitter module (EmpiricalFitter, NormalFitter, MOMFitter) is currently undocumented there, not just this PR's addition. Flagging it here rather than silently leaving the checklist item unchecked. Happy to add the doc file for the whole module as part of this PR if that's preferred, or leave it for a separate cleanup PR. Let me know which you'd rather I do.

PR checklist

For all contributions
  • I've added myself to the list of contributors
  • The PR title starts with either [ENH], [MNT], [DOC], or [BUG].
For new estimators
  • I've added the estimator to the API reference
  • 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.

@pyarchana
pyarchana marked this pull request as ready for review July 10, 2026 01:03
Comment thread .all-contributorsrc Outdated
"login": "areychana",
"name": "Archana K.",
"avatar_url": "https://avatars.githubusercontent.com/u/93970069?v=4",
"profile": "https://github.com/areychana",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think your github is wrong.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have recently changed the username, i will update it here too!

@patelchaitany

Copy link
Copy Markdown
Member

@pyarchana, add DistfitFitter under Parametric fitters in that RST file

model = dfit.model
self.dist_name_ = model["name"]
self.shape_args_ = tuple(model["arg"])
self.dist_loc_ = float(model["loc"])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this are the not documentted, Should't we document this?

@fkiraly fkiraly left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

  • the distribution returned should be existing distributions in skpro, not a new distribution without any clear features.
  • for tests, use run_test_for_class instead of an explicit dependency check

@pyarchana

Copy link
Copy Markdown
Author

Thanks!

  • the distribution returned should be existing distributions in skpro, not a new distribution without any clear features.
  • for tests, use run_test_for_class instead of an explicit dependency check

thanks!
two ques:

  • distfit's "popular" list includes distributions skpro doesn't have like genextreme and dweibull. should i limit what distfit can pick from or raise when it picks one?
  • and should _distfit_adapter.py go entirely or kept as a fallback?

…fit-fitter

# Conflicts:
#	docs/source/api_reference/distfitter.rst
#	skpro/distfitter/__init__.py
@pyarchana
pyarchana requested a review from fkiraly August 30, 2026 12:56
@pyarchana

Copy link
Copy Markdown
Author

to be clear on where this is: run_test_for_class is done and conflicts with main are resolved. the distribution mapping is waiting on your answer to the two ques above.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants