Skip to content

[BUG] Fix Levy distribution to conform with scipy interface#880

Merged
fkiraly merged 7 commits into
sktime:mainfrom
direkkakkar319-ops:issue#879-DirekKakkar-new
Mar 12, 2026
Merged

[BUG] Fix Levy distribution to conform with scipy interface#880
fkiraly merged 7 commits into
sktime:mainfrom
direkkakkar319-ops:issue#879-DirekKakkar-new

Conversation

@direkkakkar319-ops
Copy link
Copy Markdown

@direkkakkar319-ops direkkakkar319-ops commented Mar 12, 2026

Description:
Fixes #879

What was broken:
Levy.__init__ manually wrote to self.__dict__["loc"] and self.__dict__["scale"]
after the property setters already ran. This caused split-state — after set_params(),
the property returned the updated value but __dict__ retained the stale constructor
value. sklearn.clone() silently returned the wrong parameters.

What this PR does:

  • Removes the __dict__ manipulation entirely
  • Removes the unnecessary @property / setter boilerplate for loc and scale
  • Renames locmu to avoid conflict with BaseDistribution's reserved loc property
  • Follows the same pattern as Normal and Laplace in the codebase

Tests:

  • 65 Levy-specific tests: all passed
  • 89 full distribution tests: all passed

@fkiraly fkiraly changed the title [BUG] Fix Levy distribution: remove __dict__ hack that causes stale state after set_param [BUG] Fix Levy distribution to conform with scipy interface Mar 12, 2026
@fkiraly fkiraly added enhancement module:probability&simulation probability distributions and simulators labels Mar 12, 2026
@fkiraly
Copy link
Copy Markdown
Collaborator

fkiraly commented Mar 12, 2026

There were a lot of spurious methods. I removed them, the distribution is hopefully fixed now.

@direkkakkar319-ops
Copy link
Copy Markdown
Author

Hi @fkiraly! Thank you for jumping in and cleaning that up .
Glad the bug was worth fixing. I've learned a lot about how BaseDistribution handles parameters from this.

@direkkakkar319-ops
Copy link
Copy Markdown
Author

Is there anything else you'd like me to address before this PR is ready . Happy to update tests, docs, or anything else needed!

@direkkakkar319-ops
Copy link
Copy Markdown
Author

Hi @fkiraly! I noticed after your cleanup that the base class was still BaseDistribution — changed it to _ScipyAdapter so that _get_scipy_object and _get_scipy_param work correctly.

All 65 Levy tests passing now

image

@direkkakkar319-ops
Copy link
Copy Markdown
Author

Changed this

Examples -------- >>> from skpro.distributions.levy import Levy >>> dist = Levy(mu=0.0, scale=1.0) >>> dist.mean()

to this

Examples -------- >>> from skpro.distributions.levy import Levy >>> dist = Levy(mu=0.0, scale=1.0) >>> dist.mean() inf

in this file skpro/distributions/levy.py

@direkkakkar319-ops
Copy link
Copy Markdown
Author

Hi @fkiraly soon will fix to pass the test case. Inform you when is done .

@fkiraly fkiraly marked this pull request as ready for review March 12, 2026 20:19
Copy link
Copy Markdown
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.

Finished it.

@fkiraly fkiraly merged commit 61617d2 into sktime:main Mar 12, 2026
38 checks passed
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.

[BUG] Levy distribution: __dict__ manipulation breaks sklearn.clone() after set_params

3 participants