-
Notifications
You must be signed in to change notification settings - Fork 904
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
Feat/show_anomalies for multivariate #2544
Feat/show_anomalies for multivariate #2544
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2544 +/- ##
==========================================
- Coverage 94.24% 94.15% -0.09%
==========================================
Files 141 141
Lines 15466 15491 +25
==========================================
+ Hits 14576 14586 +10
- Misses 890 905 +15 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution @cnhwl, sorry for making you wait so much for a review.
The documentation of the new argument is missing, also, some refactoring of the code to reduce code duplication would be great. Other than that, it looks great!
Thank you very much for your code review and guidance! @madtoinou |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks you a lot for the changes @cnhwl , I still have some improvement to suggest
Thanks again for your help! @madtoinou 🤝 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last changes request, after this, this PR should be ready to be reviewed by @dennisbader 🚀
Happy New Year! @madtoinou @dennisbader |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the comments! LGTM, let's wait for dennis' opinion :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @cnhwl for the PR which looks nice, and @madtoinou for the review.
I saw one or two issues where I took the liberty to change some things:
- fix issue where fig, axes were not defined in case
pred_scores=None
- fix issue which raised an error when input
series
was multivariate, but anomalies and scorers arecomponent_wise=False
I'm not sure whether plot_multivariate
is the best name. To me it sounds like it should plot all components on one plot if True
(but it's the opposite). Should we rather use component_wise: bool = False
(dafault False), as we define it also for the scorers?
My last concern is about plotting all the components separately in one figure. This can get quite big for larger numbers of components. But maybe it's a bit an overkill to improve this.
At least I find that we should improve the spacing a bit between suptitle and axes.
The box for the suptitle should be fixed regardless of the number of components, whereas now it is scaled with the number of plots (see my comment).
Let me know what you think, happy to discuss further :)
Thank you very much for your code review and fix! @dennisbader Regarding parameter naming, Regarding the size of the generated image, perhaps we could prompt the user in the documentation not to add too many components that would make the image too large? |
….com/cnhwl/darts into feat/show_anomalies-for-multivariate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cnhwl, thanks for the updates. I made some last changes to address the figure size issue and some changes to the documentation.
Thanks again for the contribution and this nice PR 🚀 Always a pleasure to work together!
Checklist before merging this PR:
Fixes #2114.
Summary
I determine whether this feature is enabled by adding parameter
multivariate_plot: bool = False
toshow_anomalies()
, which is implemented in theshow_anomalies_from_scores
function.My general idea is to iterate through the components in the series and separately plot each component (including series, pred_series, pred_scores and anomalies). The following is a simple example, with the output shown below:
I would appreciate more input on how to further improve this feature, thank you very much!