Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[ENH] Skew-Normal Distribution #512

Merged
merged 16 commits into from
Jan 25, 2025
Merged

[ENH] Skew-Normal Distribution #512

merged 16 commits into from
Jan 25, 2025

Conversation

Spinachboul
Copy link
Contributor

@Spinachboul Spinachboul commented Jan 10, 2025

Reference Issues/PRs

Fixes #510

What does this implement/fix? Explain your changes.

This implementation adds a new Skew-Normal distribution as a probabilistic model in skpro. The inspiration comes from the paper (https://www.ine.pt/revstat/pdf/rs130105.pdf).

The Skew-Normal distribution captures skewness in data, improving the flexibility of predictive distributions. Scipy already implements this as scipy.stats.skewnorm, which is used as a dependency to avoid redundant code.

Does your contribution introduce a new dependency?

No new core dependencies are introduced. scipy is already a core dependency.

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, great start!

Some comments:

  • please follow the extension template carefully, all __init__ parameters should be written to self, to an attribute of the same name
  • properties should not be used for the __init__ parameters, this is due to how the base class get_params etc works
  • loc is a reserved attribute and should not be overridden. Another common name is xi, but feel free to choose anything as lon gas it is not loc.
  • code formatting tests are failing, please address

@fkiraly fkiraly added module:probability&simulation probability distributions and simulators implementing algorithms Implementing algorithms, estimators, objects native to skpro labels Jan 11, 2025
@Spinachboul
Copy link
Contributor Author

Spinachboul commented Jan 13, 2025

@fkiraly
Thanks for the suggestions
I have updated the PR by making changes in the class implementation where I have followed the extension template
But I have used BaseDistribution instead of ScipyAdapter, since that might provide more flexibility in terms of functions and general implementation.
What are your views on that??

@Spinachboul Spinachboul requested a review from fkiraly January 13, 2025 04:39
@fkiraly fkiraly changed the title Implementation of Skew-Normal Distribution [ENH] Skew-Normal Distribution Jan 13, 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!

@Spinachboul Spinachboul requested a review from fkiraly January 16, 2025 04:01
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!

Can you kindly make sure you use the extension template? Please read it carefully. For instance:

@Spinachboul Spinachboul requested a review from fkiraly January 16, 2025 17:52
@fkiraly
Copy link
Collaborator

fkiraly commented Jan 25, 2025

kindly diagnose the errors and fix them - it should be possible to spot what is wrong if you look at the logs.

Let us know if you need help.

@Spinachboul
Copy link
Contributor Author

@fkiraly
Thanks for the review
There was probably an import error, now I think it should work fine
Would request for any feedback if possible!!

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.

Great! Looks almost fine.

  • shape is a reserved parameter of distributions, so we need to use another parameter name. How about xi, omega, alpha like on wikipedia?
  • for the default list of maintainers, do not set the maintainer tag.

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.

Fixed this to have it in the release.

@fkiraly fkiraly merged commit 65ad9cd into sktime:main Jan 25, 2025
36 checks passed
@Spinachboul
Copy link
Contributor Author

@fkiraly
Thanks for the changes! Meanwhile I was also thinking of having changes in the .rst pages :
image
Currently they appear like this
Maybe if we could update them , have their proper documentation.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
implementing algorithms Implementing algorithms, estimators, objects native to skpro module:probability&simulation probability distributions and simulators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH] Skew-Normal Distribution
2 participants