Skip to content

[BUG] Fix incorrect handling of negative n in BaseDistribution.head()#923

Merged
fkiraly merged 3 commits into
sktime:mainfrom
mohityadav8:fix-head-negative-n-test
Apr 4, 2026
Merged

[BUG] Fix incorrect handling of negative n in BaseDistribution.head()#923
fkiraly merged 3 commits into
sktime:mainfrom
mohityadav8:fix-head-negative-n-test

Conversation

@mohityadav8
Copy link
Copy Markdown
Contributor

Reference Issues/PRs

Fixes #894

What does this implement/fix? Explain your changes.

This PR fixes incorrect handling of negative values of n in BaseDistribution.head().

The current implementation uses:

n = N - n

When n is negative, this produces incorrect results.

Example:
If N = 10 and n = -2, the current logic computes:

n = 10 - (-2) = 12

This value is then clipped to 10 and returns all rows instead of returning
all rows except the last two.

The documented behavior states:
"For negative n, returns all rows except the last n."

This PR updates the logic to:

n = N + n

which correctly computes the intended number of rows.

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

No new dependencies were introduced.

What should a reviewer concentrate their feedback on?

  • Correctness of the updated logic in BaseDistribution.head()
  • Whether the behavior matches the documented behavior for negative n

Did you add any tests for the change?

No new tests were added. Existing tests pass successfully.

Any other comments?

All existing tests pass locally after this change.

PR checklist

  • Code builds successfully
  • Tests pass locally
  • No new dependencies added
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.

@mohityadav8
Copy link
Copy Markdown
Contributor Author

Previously worked on in #895

@mohityadav8
Copy link
Copy Markdown
Contributor Author

Hi @felipeangelimvieira and @fkiraly
It looks like the workflows are awaiting maintainer approval and the PR also requires a code owner review.
Could you please approve the workflows and review the PR when convenient?
Thanks!

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.

I think you haven't actually tried to run the code because it does not run if you try.

Is this AI generated copy-paste?

@fkiraly fkiraly added bug module:probability&simulation probability distributions and simulators AI overuse suspected labels Apr 4, 2026
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.

I fixed it

@fkiraly fkiraly merged commit f961a7a into sktime:main Apr 4, 2026
38 checks passed
@mohityadav8
Copy link
Copy Markdown
Contributor Author

mohityadav8 commented May 5, 2026

Thanks☺️ @fkiraly

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

Labels

AI overuse suspected bug module:probability&simulation probability distributions and simulators

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] BaseDistribution.head returns incorrect results for negative n

2 participants