Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix quantile detector when low/high threshold are the same #1553

Merged
merged 10 commits into from Feb 16, 2023

Conversation

julien12234
Copy link
Contributor

@julien12234 julien12234 commented Feb 10, 2023

Fixes #1551. Fix the issue raised by philippspengler:

Describe the bug
The QuantileDetector class raises an error during fitting when the time series has low variation.

To Reproduce

series = TimeSeries.from_series(pd.Series([96.0, 96.0, 96.0, 96.0, 96.0, 96.0, 96.0, 96.0, 96.0, 96.0, 96.0, 96.0, 96.0, 96.0, 96.0, 96.0, 96.0, 96.0, 96.0, 96.0, 93.0]))
quantile_detector = QuantileDetector(low_quantile=0.05, high_quantile=0.95)
quantile_detector.fit(series)
Expected behavior
Expect the quantile detector to fit without an error.

System (please complete the following information):

Python version: 3.8.5
darts version 0.23.1

Solution:

The problemed occurred when the low and high quantiles corresponded to the same number, and when calling the threshold detector, an error was raised as it expected two different values. The condition was removed.

@julien12234 julien12234 added this to In review in darts via automation Feb 10, 2023
Copy link
Collaborator

@dennisbader dennisbader left a comment

Choose a reason for hiding this comment

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

👍 Nice, thanks! Could you also add a unit test to check that?

Btw you can reference issues like this: #1551
And you can write "fixes #1551" in your PR description to automatically close the issue as soon as the PR is merged :)

darts/ad/detectors/threshold_detector.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Feb 10, 2023

Codecov Report

Base: 94.09% // Head: 94.02% // Decreases project coverage by -0.07% ⚠️

Coverage data is based on head (d4dd59e) compared to base (e1c8d34).
Patch coverage: 85.18% of modified lines in pull request are covered.

❗ Current head d4dd59e differs from pull request most recent head 57d8ab2. Consider uploading reports for the commit 57d8ab2 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1553      +/-   ##
==========================================
- Coverage   94.09%   94.02%   -0.07%     
==========================================
  Files         125      125              
  Lines       11071    11081      +10     
==========================================
+ Hits        10417    10419       +2     
- Misses        654      662       +8     
Impacted Files Coverage Δ
darts/ad/detectors/quantile_detector.py 100.00% <ø> (ø)
darts/ad/detectors/threshold_detector.py 100.00% <ø> (ø)
darts/models/forecasting/prophet_model.py 92.59% <82.60%> (-1.70%) ⬇️
darts/ad/scorers/kmeans_scorer.py 100.00% <100.00%> (ø)
darts/datasets/__init__.py 100.00% <100.00%> (ø)
darts/utils/timeseries_generation.py 96.15% <100.00%> (ø)
darts/utils/data/tabularization.py 99.27% <0.00%> (-0.73%) ⬇️
darts/timeseries.py 92.14% <0.00%> (-0.23%) ⬇️
darts/ad/anomaly_model/filtering_am.py 91.93% <0.00%> (-0.13%) ⬇️
...arts/models/forecasting/torch_forecasting_model.py 89.52% <0.00%> (-0.05%) ⬇️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

julien12234 and others added 2 commits February 10, 2023 15:15
Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>
Copy link
Collaborator

@dennisbader dennisbader left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks 🚀

@hrzn hrzn merged commit f2efd00 into master Feb 16, 2023
darts automation moved this from In review to Done Feb 16, 2023
@madtoinou madtoinou deleted the fix/quantile_detector branch March 12, 2023 14:48
@dennisbader dennisbader moved this from Done to Released in darts May 23, 2023
alexcolpitts96 pushed a commit to alexcolpitts96/darts that referenced this pull request May 31, 2023
)

* fix quantile detector when low/high threshold are the same

* add test

* Update darts/ad/detectors/threshold_detector.py

Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>

* change syntax

* pre commit

* pre commit

---------

Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>
Co-authored-by: madtoinou <32447896+madtoinou@users.noreply.github.com>
Co-authored-by: Julien Herzen <julien@unit8.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
darts
Released
Development

Successfully merging this pull request may close these issues.

[BUG] Series with Similar Values Produce ValueError in Quantile Detection
5 participants