Skip to content

[BUG] Fix missing negative sign in SPLL metric calculates negative loss for censored data#1032

Merged
fkiraly merged 1 commit into
sktime:mainfrom
KaranSinghDev:bugfix/spll-sign-error
Apr 12, 2026
Merged

[BUG] Fix missing negative sign in SPLL metric calculates negative loss for censored data#1032
fkiraly merged 1 commit into
sktime:mainfrom
KaranSinghDev:bugfix/spll-sign-error

Conversation

@KaranSinghDev
Copy link
Copy Markdown
Contributor

@KaranSinghDev KaranSinghDev commented Apr 6, 2026

What does this implement/fix? Explain your changes.

Fixes a mathematical sign error in the SPLL (Survival Process Logarithmic Loss) metric calculation for censored data.

In _spll.py, the docstring correctly defines the loss for a censored instance ($\Delta = 1$) as:
$L = - \Delta \log d.S(y)$

However, the implementation was missing negative sign for the discrete term:

# Before
disc_term = np.log(1 - y_pred.cdf(y_true)) * C_true.to_numpy()

Because $S(y) = 1 - CDF \le 1$, its logarithm is inherently negative. Without the leading minus sign, the metric was returning a negative loss for censored instances. Since lower_is_better=True, this should inadvertently reward models that assigned lower survival probabilities to censored data (e.g., an Exponential model predicting $S(y)=e^{-1}$ was returning a score of -1.0 instead of 1.0).

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

No

What should a reviewer concentrate their feedback on?

Verification of the mathematical correction in _spll.py.

Did you add any tests for the change?

No, I have made a regression test but would like to discuss whether to add seperately or append in any of the tests .

Any other comments?

PR checklist

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.

@KaranSinghDev KaranSinghDev marked this pull request as draft April 6, 2026 12:23
@KaranSinghDev KaranSinghDev force-pushed the bugfix/spll-sign-error branch from cf4f2e0 to bac662f Compare April 6, 2026 12:37
@KaranSinghDev KaranSinghDev marked this pull request as ready for review April 6, 2026 12:37
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.

Yes, this is indeed a typo. Thanks for fixing!

@fkiraly fkiraly added bug module:metrics&benchmarking metrics and benchmarking modules labels Apr 9, 2026
@fkiraly fkiraly merged commit a5a93cf into sktime:main Apr 12, 2026
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug module:metrics&benchmarking metrics and benchmarking modules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants