Skip to content

[ENH] Add pdfnorm to test coverage #712

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.

Sign up for GitHub

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

Jump to bottom
Open
arnavk23 wants to merge 13 commits into
base: main
Choose a base branch
from
Open

[ENH] Add pdfnorm to test coverage #712

arnavk23 wants to merge 13 commits into from

Conversation

@arnavk23
Copy link
Contributor

@arnavk23 arnavk23 commented Jan 24, 2026

Reference Issues/PRs

Fixes #205.

What does this implement/fix? Explain your changes.

  • Adds missing test coverage for BaseDistribution.pdfnorm fallback by including 'pdfnorm' in METHODS_SCALAR and METHODS_SCALAR_POS.
  • Fixes pdfnorm method to handle scalar distributions correctly, returning float instead of DataFrame for ndim=0 cases.

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

No.

What should a reviewer concentrate their feedback on?

  • Correctness of pdfnorm scalar handling
  • Appropriateness of adding pdfnorm to test methods

Did you add any tests for the change?

No new tests added, but enabled existing TestAllDistributions infrastructure to cover pdfnorm fallback.

Any other comments?

PR checklist

For all contributions
  • 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.

@arnavk23
Fix issue sktime#205: Add pdfnorm to test coverage
9453a7d 
- Add 'pdfnorm' to METHODS_SCALAR in test_all_distrs.py so that the fallback implementation gets tested
- Add 'pdfnorm' to METHODS_SCALAR_POS since it returns non-negative values
- Fix pdfnorm method to handle scalar distributions correctly by returning float instead of DataFrame for ndim=0
- Fix a=1 special case in pdfnorm to return 1.0 for scalar distributions
@arnavk23
Fix test failure: skip pdfnorm test when pdf not available
ce47dbf 
- Added check in test_methods_scalar to skip pdfnorm testing for distributions that don't have pdf capability
- This prevents failures on TransformedDistribution which has pdfnorm in approx capabilities but not pdf
@arnavk23
Merge branch 'sktime:main' into fix/issue-205-pdfnorm-fallback
ffb3830
fkiraly
fkiraly reviewed Jan 27, 2026
View reviewed changes
fkiraly
fkiraly reviewed Jan 27, 2026
View reviewed changes
fkiraly
fkiraly reviewed Jan 27, 2026
View reviewed changes
@fkiraly
Copy link
Collaborator

fkiraly commented Jan 27, 2026

hm... something is causing exceptions now, though?

@fkiraly
Copy link
Collaborator

fkiraly commented Jan 28, 2026

still. Any idea what is going on?

@fkiraly fkiraly added enhancement module:probability&simulation probability distributions and simulators labels Jan 28, 2026
@arnavk23
Fix TransformedDistribution PDF implementation
966f9b4 
- Add _pdf method supporting exact computation via change-of-variables formula when inverse transform exists
- Add approximate PDF computation via numerical differentiation of CDF when inverse transform is not available
- Update capability tags to include PDF availability
- Override pdfnorm with reduced sample size (10) to prevent timeouts in tests
- Ensure PDF values are non-negative using absolute values where necessary
- Handle both scalar and array cases with proper DataFrame indexing
@arnavk23
Fix test_deps_info to use correct availability check
53447a5 
- Replace _check_soft_dependencies with direct import check for availability
- This fixes issues with packages that fail import due to version incompatibilities (e.g., numba with NumPy 2.4)
- Ensure test accurately reflects _get_deps_info behavior
@arnavk23
Apply precommit formatting and fixes
ba8141d 
- Fix missing method definition for _sample in TransformedDistribution
- Add stacklevel=2 to warnings.warn call
- Apply black formatting to _transformed.py
- Apply pyupgrade changes to estimators.py
@arnavk23
Add missing docstrings to parametric estimators
f38a83f 
- Add module docstring
- Fix class docstrings to end with period
- Add docstrings to fit and predict methods
- Add docstrings to __str__ and __repr__ magic methods
@arnavk23
Copy link
Contributor Author

arnavk23 commented Jan 28, 2026

still. Any idea what is going on?

Fixed now.

@arnavk23 arnavk23 requested a review from fkiraly January 28, 2026 22:47
fkiraly
fkiraly requested changes Jan 30, 2026
View reviewed changes
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 explain why you are:

  • changing _pdf of the TransformedDistribution?
  • changing test_deps_info?

@arnavk23
Copy link
Contributor Author

arnavk23 commented Jan 30, 2026

Thanks!

Thanks for reviewing!

  1. I added a _pdf method because pdfnorm depends on it by calling self.pdf(self.sample()) internally. The implementation uses the change of variables formula when inverse_transform is available, otherwise falls back to numerical differentiation of the CDF. I also added a custom pdfnorm with reduced sample size (10 vs ~1000) to avoid test timeouts.

  2. This fixes a bug where the test was checking availability using the aliased package name (e.g., "scikit-learn") instead of the import name (e.g., "sklearn"). The fix uses importlib.import_module() with the correct import name to properly detect available dependencies.

@arnavk23 arnavk23 requested a review from fkiraly January 30, 2026 22:07
@fkiraly
Copy link
Collaborator

fkiraly commented Feb 2, 2026

can you split the change to test_deps_info off this PR and report the bug please? Or is it somehow related to the present PR?

@arnavk23
Copy link
Contributor Author

arnavk23 commented Feb 2, 2026

can you split the change to test_deps_info off this PR and report the bug please? Or is it somehow related to the present PR?

All changes in this pr are either related to the issue or were added due to failing checks (parameter/_estimator, test_show_versions). These failing checks files were causing issues in the estimator overview pr as well, causing code format failing check there.

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

Reviewers

@felipeangelimvieira felipeangelimvieira Awaiting requested review from felipeangelimvieira felipeangelimvieira is a code owner

@SaiRevanth25 SaiRevanth25 Awaiting requested review from SaiRevanth25 SaiRevanth25 is a code owner

@fkiraly fkiraly Awaiting requested review from fkiraly fkiraly is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

enhancement module:probability&simulation probability distributions and simulators

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] check missing test coverage for BaseDistribution.pdfnorm fallback

2 participants

@arnavk23 @fkiraly