Skip to content

ENH: fix HalfCauchy scipy mapping and add dedicated tests (towards #22)#954

Open
kunal14901 wants to merge 1 commit intosktime:mainfrom
kunal14901:enh/halfcauchy-distribution
Open

ENH: fix HalfCauchy scipy mapping and add dedicated tests (towards #22)#954
kunal14901 wants to merge 1 commit intosktime:mainfrom
kunal14901:enh/halfcauchy-distribution

Conversation

@kunal14901
Copy link

@kunal14901 kunal14901 commented Mar 16, 2026

Reference Issues/PRs

See #22

What does this implement/fix? Explain your changes.

This PR fixes scipy parameter mapping for HalfCauchy and adds dedicated regression tests.

Changes made:

  • Updated HalfCauchy._get_scipy_param to pass beta as scipy keyword argument scale.

    • before: return [beta], {}
    • after: return [], {"scale": beta}
  • Added skpro/distributions/tests/test_halfcauchy.py with focused tests for:

    • scalar parity against scipy.stats.halfcauchy for pdf, cdf, and ppf
    • support behavior for negative inputs (x < 0)
    • broadcasting and shape/index/columns behavior with DataFrame inputs

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

No, this PR does not introduce any new dependency.

What should a reviewer concentrate their feedback on?

  • Correctness of scipy adapter mapping in _get_scipy_param
  • Correctness/style of the new HalfCauchy tests
  • Any preferred convention for distribution-specific test modules

Did you add any tests for the change?

Yes.

Added:

  • skpro/distributions/tests/test_halfcauchy.py

Validation:

  • targeted test file passed
  • test_all_distrs.py filtered to HalfCauchy passed (103 selected tests)

Any other comments?

Submitted as part of my ESoC 2026 application contribution towards #22.
Happy to iterate quickly on feedback.

PR checklist

For all contributions
  • I've added myself to the list of contributors with any new badges I've earned
  • The PR title starts with either [ENH], [MNT], [DOC], or [BUG].
For new estimators
  • Not applicable (this PR fixes adapter mapping and adds tests for an existing distribution)
  • Not applicable
  • Not applicable

Copilot AI review requested due to automatic review settings March 16, 2026 20:00
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes the SciPy adapter parameter mapping for HalfCauchy so that beta is passed as SciPy’s scale kwarg (instead of being interpreted as loc via a positional arg), and adds focused regression tests to prevent reintroducing the issue.

Changes:

  • Corrected HalfCauchy._get_scipy_param to return {"scale": beta} as keyword arguments.
  • Added test_halfcauchy.py to verify scalar parity with scipy.stats.halfcauchy (pdf, cdf, ppf), support behavior for x < 0, and DataFrame broadcasting/shape preservation.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
skpro/distributions/halfcauchy.py Fixes the SciPy parameter mapping so beta is passed as scale.
skpro/distributions/tests/test_halfcauchy.py Adds regression tests covering scalar parity, support for negative inputs, and DataFrame broadcasting/shape behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

@kunal14901
Copy link
Author

kunal14901 commented Mar 16, 2026

Hi @fkiraly and @felipeangelimvieira , thanks for reviewing.
This PR is a focused bug fix towards #22:

fixes HalfCauchy scipy parameter mapping (scale=beta)
adds dedicated regression tests for scipy parity and broadcast/shape behavior
All targeted tests pass locally, and I am happy to make any changes quickly

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants