fix: extend zero_division parameter to percentage and range-based metrics#3122
Open
mahimn01 wants to merge 1 commit into
Open
fix: extend zero_division parameter to percentage and range-based metrics#3122mahimn01 wants to merge 1 commit into
mahimn01 wants to merge 1 commit into
Conversation
…rics Percentage and range-based metrics (`wmape`, `ope`, `arre`, `marre`, `coefficient_of_variation`) previously either raised a hard `ValueError` or silently returned `nan`/`inf` when their denominator was zero. This made batch evaluation pipelines brittle for constant or all-zero components. Mirrors the `zero_division` design introduced in unit8co#3059 for the scaled error family: `"warn"` (default) returns `np.nan` and emits a warning, `"raise"` preserves the legacy `ValueError`. A new `_safe_pct_divide` helper sits next to `_safe_scaled_divide`; the two differ only in fill semantics — percentage metrics multiply the ratio by 100 so a `1.0` fill for the 0/0 case (the scaled-metric "on par with naive") would surface as `100 %` error and be misleading, hence `np.nan` instead. Two adjacent bugs surface and are fixed in the same change: * `ope` previously checked `sum > 0` and rejected `actual_series` with a strictly negative sum (e.g. financial return series). The check is now `sum != 0` via the helper. * `wmape`'s docstring claimed `ValueError if actual_series contains some zeros`, but the implementation divides by `sum(|y_true|)` and only the all-zero case ever triggered the path. Docstring corrected. The CHANGELOG entry for the parameter addition carries the breaking- change marker per the convention discussed in unit8co#3080 (the post-mortem on unit8co#3059), since the default behavior flips from raising to warning. Adds a parametrized regression test covering all five metrics and an explicit OPE-with-negative-sum test. Existing `test_ope_zero` and the arre/marre legacy raise check are updated to opt into the legacy behavior with `zero_division="raise"`.
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.
Checklist before merging this PR:
Mirrors the design of #3059 for the percentage / range-based metric family. Adopts the breaking-change 🔴 marker convention discussed in #3080.
Summary
Percentage and range-based metrics (
wmape,ope,arre,marre,coefficient_of_variation) previously either raised a hardValueErroror silently returnednan/infwhen their denominator was zero. This made batch evaluation pipelines brittle for constant or all-zero components.This PR extends the
zero_divisionparameter pattern introduced in #3059 to these five metrics:"warn"(default) — returnsnp.nanand emits a warning."raise"— preserves the legacyValueError.A new
_safe_pct_dividehelper sits next to_safe_scaled_divideindarts/metrics/utils.py. The two differ only in fill semantics: percentage metrics multiply the ratio by 100, so a1.0fill for the 0/0 case (the scaled-metric "on par with naive") would surface as100 %error and be misleading — hencenp.naninstead.Two adjacent bugs are fixed in the same change:
opepreviously checkedsum > 0and rejectedactual_serieswith a strictly negative sum (e.g. financial return series). The check is nowsum != 0via the helper.wmape's docstring claimedValueError if actual_series contains some zeros, but the implementation divides bysum(|y_true|)and only the all-zero case ever triggered any path. Docstring corrected.Behavior change
"warn"np.nanand logs a warning (previously: silent NaN/inf, or hardValueError)"raise"ValueError(legacy behavior preserved as opt-in)actual_seriesinopePer the discussion in #3080, the CHANGELOG entry for the parameter addition carries the breaking-change 🔴 marker because the default behavior flips from raising to warning.
Test plan
test_pct_metrics_zero_divisioncovering all 5 metrics: warn-mode returns NaN with warning, raise-mode raisesValueError, invalid string rejected, healthy inputs unaffected.test_ope_accepts_negative_sumverifying thesum != 0fix using a series with a strictly negative sum.test_ope_zeroandtest_arreto opt into legacy behavior withzero_division="raise".uv run pytest darts/tests/metrics/test_metrics.py→ 461 passed (was 455, +6 new).uv run pre-commit run --files <changed>→ all hooks pass (ruff, ruff-format, trailing whitespace, end-of-file).Other Information
Files changed (+208/-31):
darts/metrics/utils.py—_safe_pct_dividehelperdarts/metrics/metrics.py— 5 metrics extendeddarts/tests/metrics/test_metrics.py— 6 new tests + 2 legacy-test updatesCHANGELOG.md— entries under**Fixed**(the bug fixes) and**Improved**(the new parameter, with 🔴 marker)