[BUG] fix scipy scale mapping for HalfNormal/HalfLogistic with regression tests (towards #22)#957
Open
kunal14901 wants to merge 2 commits intosktime:mainfrom
Open
Conversation
kunal14901
requested review from
felipeangelimvieira and
fkiraly
as code owners
Author
There was a problem hiding this comment.
Pull request overview
Fixes SciPy parameter mapping for half-distributions by passing scale as a keyword argument (avoiding incorrect positional binding), and adds regression tests to ensure parity with SciPy for scalar and broadcasted cases.
Changes:
- Update
_get_scipy_paramforHalfNormal,HalfLogistic, andHalfCauchyto return{"scale": ...}as kwargs instead of positional args. - Add regression tests comparing
pdf/cdf/ppfagainst SciPy for scalar inputs. - Add broadcast/shape/index/columns regression checks for DataFrame-backed evaluations.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| skpro/distributions/halfnormal.py | Fix SciPy parameter mapping to use scale= keyword for halfnorm. |
| skpro/distributions/halflogistic.py | Fix SciPy parameter mapping to use scale= keyword for halflogistic. |
| skpro/distributions/halfcauchy.py | Fix SciPy parameter mapping to use scale= keyword for halfcauchy. |
| skpro/distributions/tests/test_half_distributions.py | Add regression tests for HalfNormal/HalfLogistic scalar parity and broadcast behavior. |
| skpro/distributions/tests/test_halfcauchy.py | Add regression tests for HalfCauchy scalar parity and broadcast 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.
Comment on lines
68
to
+70
| def _get_scipy_param(self): | ||
| beta = self._bc_params["beta"] | ||
| return [beta], {} | ||
| return [], {"scale": beta} |
Comment on lines
+19
to
+48
| def test_halfnormal_scalar_matches_scipy_scale(): | ||
| """HalfNormal scalar pdf/cdf/ppf should match scipy with scale=sigma.""" | ||
| sigma = 2.5 | ||
| dist = HalfNormal(sigma=sigma) | ||
|
|
||
| x = 1.7 | ||
| p = 0.8 | ||
|
|
||
| assert_allclose(dist.pdf(x), halfnorm.pdf(x, scale=sigma), rtol=1e-12) | ||
| assert_allclose(dist.cdf(x), halfnorm.cdf(x, scale=sigma), rtol=1e-12) | ||
| assert_allclose(dist.ppf(p), halfnorm.ppf(p, scale=sigma), rtol=1e-12) | ||
|
|
||
|
|
||
| @pytest.mark.skipif( | ||
| not run_test_module_changed("skpro.distributions"), | ||
| reason="run only if skpro.distributions has been changed", | ||
| ) | ||
| def test_halflogistic_scalar_matches_scipy_scale(): | ||
| """HalfLogistic scalar pdf/cdf/ppf should match scipy with scale=beta.""" | ||
| beta = 1.8 | ||
| dist = HalfLogistic(beta=beta) | ||
|
|
||
| x = 1.3 | ||
| p = 0.7 | ||
|
|
||
| assert_allclose(dist.pdf(x), halflogistic.pdf(x, scale=beta), rtol=1e-12) | ||
| assert_allclose(dist.cdf(x), halflogistic.cdf(x, scale=beta), rtol=1e-12) | ||
| assert_allclose(dist.ppf(p), halflogistic.ppf(p, scale=beta), rtol=1e-12) | ||
|
|
||
|
|
Comment on lines
+19
to
+49
| def test_halfnormal_scalar_matches_scipy_scale(): | ||
| """HalfNormal scalar pdf/cdf/ppf should match scipy with scale=sigma.""" | ||
| sigma = 2.5 | ||
| dist = HalfNormal(sigma=sigma) | ||
|
|
||
| x = 1.7 | ||
| p = 0.8 | ||
|
|
||
| assert_allclose(dist.pdf(x), halfnorm.pdf(x, scale=sigma), rtol=1e-12) | ||
| assert_allclose(dist.cdf(x), halfnorm.cdf(x, scale=sigma), rtol=1e-12) | ||
| assert_allclose(dist.ppf(p), halfnorm.ppf(p, scale=sigma), rtol=1e-12) | ||
|
|
||
|
|
||
| @pytest.mark.skipif( | ||
| not run_test_module_changed("skpro.distributions"), | ||
| reason="run only if skpro.distributions has been changed", | ||
| ) | ||
| def test_halflogistic_scalar_matches_scipy_scale(): | ||
| """HalfLogistic scalar pdf/cdf/ppf should match scipy with scale=beta.""" | ||
| beta = 1.8 | ||
| dist = HalfLogistic(beta=beta) | ||
|
|
||
| x = 1.3 | ||
| p = 0.7 | ||
|
|
||
| assert_allclose(dist.pdf(x), halflogistic.pdf(x, scale=beta), rtol=1e-12) | ||
| assert_allclose(dist.cdf(x), halflogistic.cdf(x, scale=beta), rtol=1e-12) | ||
| assert_allclose(dist.ppf(p), halflogistic.ppf(p, scale=beta), rtol=1e-12) | ||
|
|
||
|
|
||
| @pytest.mark.skipif( |
| x = 1.7 | ||
| p = 0.8 | ||
|
|
||
| assert_allclose(dist.pdf(x), halfnorm.pdf(x, scale=sigma), rtol=1e-12) |
| pdf = dist.pdf(x) | ||
| cdf = dist.cdf(x) | ||
| ppf = dist.ppf(p) | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reference Issues/PRs
See #22
What does this implement/fix? Explain your changes.
This PR fixes scipy parameter mapping semantics for two half-distributions and adds dedicated regression tests.
Changes:
HalfNormal._get_scipy_paramreturn [sigma], {}return [], {"scale": sigma}HalfLogistic._get_scipy_paramreturn [beta], {}return [], {"scale": beta}These changes align parameter passing with scipy keyword semantics and avoid incorrect positional binding behavior.
Additionally, this PR adds:
skpro/distributions/tests/test_half_distributions.pyThe new tests validate:
pdf,cdf,ppf) forHalfNormalandHalfLogisticDoes your contribution introduce a new dependency? If yes, which one?
No new dependency is introduced.
What should a reviewer concentrate their feedback on?
Did you add any tests for the change?
Yes.
Added:
skpro/distributions/tests/test_half_distributions.pyValidation performed locally:
test_half_distributions.pypassedtest_all_distrs.pyforHalfNormal/HalfLogisticpassedAny other comments?
Submitted as part of my ESoC 2026 contributions towards #22.
Happy to iterate quickly on review feedback.
PR checklist
For all contributions
For new estimators