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

Anomaly Detection (anomaly model, scorer, detector, aggregator) #1256

Merged
merged 151 commits into from Dec 22, 2022

Conversation

julien12234
Copy link
Contributor

Fixes #.

Summary

Other Information

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@julien12234 julien12234 marked this pull request as ready for review October 4, 2022 12:55
Copy link
Contributor

@hrzn hrzn left a comment

Choose a reason for hiding this comment

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

Just a first round of comments, mostly about formatting for now :)

darts/anomaly_detection/anomaly_model.py Outdated Show resolved Hide resolved
darts/anomaly_detection/anomaly_model.py Outdated Show resolved Hide resolved
darts/anomaly_detection/anomaly_model.py Outdated Show resolved Hide resolved
darts/anomaly_detection/anomaly_model.py Outdated Show resolved Hide resolved
darts/anomaly_detection/anomaly_model.py Outdated Show resolved Hide resolved
test_jupyter.ipynb Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Oct 10, 2022

Codecov Report

Base: 93.83% // Head: 93.93% // Increases project coverage by +0.10% 🎉

Coverage data is based on head (82b65cb) compared to base (919f214).
Patch coverage: 94.94% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1256      +/-   ##
==========================================
+ Coverage   93.83%   93.93%   +0.10%     
==========================================
  Files          94      122      +28     
  Lines        9673    10709    +1036     
==========================================
+ Hits         9077    10060     +983     
- Misses        596      649      +53     
Impacted Files Coverage Δ
darts/ad/anomaly_model/anomaly_model.py 80.76% <80.76%> (ø)
darts/ad/utils.py 90.86% <90.86%> (ø)
darts/ad/anomaly_model/filtering_am.py 91.93% <91.93%> (ø)
darts/ad/anomaly_model/forecasting_am.py 93.45% <93.45%> (ø)
darts/ad/scorers/scorers.py 95.13% <95.13%> (ø)
darts/ad/aggregators/aggregators.py 95.77% <95.77%> (ø)
darts/ad/detectors/detectors.py 96.07% <96.07%> (ø)
darts/ad/__init__.py 100.00% <100.00%> (ø)
darts/ad/aggregators/__init__.py 100.00% <100.00%> (ø)
darts/ad/aggregators/and_aggregator.py 100.00% <100.00%> (ø)
... and 23 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.

.gitignore Outdated Show resolved Hide resolved
darts/anomaly_detection/anomaly_model.py Outdated Show resolved Hide resolved
darts/anomaly_detection/anomaly_model.py Outdated Show resolved Hide resolved
Copy link
Contributor

@hrzn hrzn left a comment

Choose a reason for hiding this comment

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

I've made a round of comments.

Overall, I think it's a great start and taking shape. Here are the most important points:

  • Please, think of what happens both for (i) multivariate series and (ii) multiple series. It seems to me that the code is not general enough in few places about that.
  • Unless I'm missing something, I think most scorers (e.g. L2, but also KMeans and others) should act in the original D-dimensional space for D-dimensional multivariate series, and always return univariate series (I don't think it's really done like that currently).
  • Somehow, I would expect KMeansScorer and other similar scorers to work over certain rolling windows (kind of what you have for Wasserstein). This is probably where the characteristic time length should play (rather than in some functions keeping e.g. the max values over certain windows as a postprocessing step for all scorers - I would suggest removing this).
  • I would let the scorers compute the distance themselves if they need it, rather than feeding them with (some variants) of residuals.
  • I haven't looked at Detectors yet
  • I have pushed a couple of commits with small improvements (mainly to docstrings), but more corrections need to be made there

darts/ad/anomaly_model.py Outdated Show resolved Hide resolved
darts/ad/anomaly_model.py Outdated Show resolved Hide resolved
darts/ad/anomaly_model.py Outdated Show resolved Hide resolved
darts/ad/score.py Outdated Show resolved Hide resolved
darts/ad/score.py Outdated Show resolved Hide resolved
darts/ad/scorers.py Outdated Show resolved Hide resolved
darts/ad/scorers.py Outdated Show resolved Hide resolved
darts/ad/scorers.py Outdated Show resolved Hide resolved
darts/ad/anomaly_model.py Outdated Show resolved Hide resolved
darts/ad/anomaly_model.py Outdated Show resolved Hide resolved
hrzn and others added 18 commits December 22, 2022 15:29
Co-authored-by: eliane-maalouf <112691612+eliane-maalouf@users.noreply.github.com>
Co-authored-by: eliane-maalouf <112691612+eliane-maalouf@users.noreply.github.com>
Co-authored-by: eliane-maalouf <112691612+eliane-maalouf@users.noreply.github.com>
Co-authored-by: eliane-maalouf <112691612+eliane-maalouf@users.noreply.github.com>
Co-authored-by: eliane-maalouf <112691612+eliane-maalouf@users.noreply.github.com>
Co-authored-by: eliane-maalouf <112691612+eliane-maalouf@users.noreply.github.com>
Co-authored-by: eliane-maalouf <112691612+eliane-maalouf@users.noreply.github.com>
Co-authored-by: eliane-maalouf <112691612+eliane-maalouf@users.noreply.github.com>
Co-authored-by: eliane-maalouf <112691612+eliane-maalouf@users.noreply.github.com>
Co-authored-by: eliane-maalouf <112691612+eliane-maalouf@users.noreply.github.com>
Co-authored-by: eliane-maalouf <112691612+eliane-maalouf@users.noreply.github.com>
Co-authored-by: eliane-maalouf <112691612+eliane-maalouf@users.noreply.github.com>
Co-authored-by: eliane-maalouf <112691612+eliane-maalouf@users.noreply.github.com>
Co-authored-by: eliane-maalouf <112691612+eliane-maalouf@users.noreply.github.com>
Co-authored-by: eliane-maalouf <112691612+eliane-maalouf@users.noreply.github.com>
Co-authored-by: eliane-maalouf <112691612+eliane-maalouf@users.noreply.github.com>
Co-authored-by: eliane-maalouf <112691612+eliane-maalouf@users.noreply.github.com>
Co-authored-by: eliane-maalouf <112691612+eliane-maalouf@users.noreply.github.com>
darts/ad/utils.py Outdated Show resolved Hide resolved
darts/ad/detectors/quantile_detector.py Outdated Show resolved Hide resolved
darts/ad/scorers/nll_exponential_scorer.py Show resolved Hide resolved
Copy link
Contributor

@eliane-maalouf eliane-maalouf left a comment

Choose a reason for hiding this comment

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

AD is a great addition to the Darts @julien12234 and @hrzn . Thanks for this colossal work :)

darts/ad/scorers/nll_cauchy_scorer.py Show resolved Hide resolved
darts/ad/scorers/pyod_scorer.py Outdated Show resolved Hide resolved
darts/ad/scorers/wasserstein_scorer.py Show resolved Hide resolved
hrzn and others added 3 commits December 22, 2022 16:34
Co-authored-by: eliane-maalouf <112691612+eliane-maalouf@users.noreply.github.com>
@hrzn hrzn changed the title Anomaly Detection First Version (anomaly_model, scorer, detector) Anomaly Detection (anomaly model, scorer, detector, aggregator) Dec 22, 2022
Copy link
Contributor

@hrzn hrzn left a comment

Choose a reason for hiding this comment

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

🥳

@hrzn hrzn merged commit 67edbeb into master Dec 22, 2022
@madtoinou madtoinou deleted the feat/anomaly_detection_API branch July 5, 2023 21:53
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.

None yet

5 participants