Skip to content

Add SrCnn Anomaly Detector #3693

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
May 31, 2019
Merged

Add SrCnn Anomaly Detector #3693

merged 14 commits into from
May 31, 2019

Conversation

mengaims
Copy link
Contributor

@mengaims mengaims commented May 9, 2019 •
edited by codemzs
Loading

Add SRCNN algorithm to TimSeries Anomaly Detection

fixes #3799

@dnfclas
Copy link

dnfclas commented May 9, 2019 •
edited
Loading

CLA assistant check
All CLA requirements met.

@codemzs codemzs self-requested a review May 9, 2019 15:16
@codecov
Copy link

codecov bot commented May 9, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@18a2615). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master    #3693   +/-   ##
=========================================
  Coverage          ?   72.57%           
=========================================
  Files             ?      811           
  Lines             ?   146001           
  Branches          ?    16279           
=========================================
  Hits              ?   105958           
  Misses            ?    35620           
  Partials          ?     4423
Flag Coverage Δ
#Debug 72.57% <0%> (?)
#production 68.03% <0%> (?)
#test 89.03% <ø> (?)
Impacted Files Coverage Δ
src/Microsoft.ML.TimeSeries/ExtensionsCatalog.cs 87.5% <0%> (ø)
...crosoft.ML.TimeSeries/SrCnnAnomalyDetectionBase.cs 0% <0%> (ø)
src/Microsoft.ML.TimeSeries/SrCnnTransformBase.cs 0% <0%> (ø)
...rc/Microsoft.ML.TimeSeries/SRCNNAnomalyDetector.cs 0% <0%> (ø)

@najeeb-kazmi najeeb-kazmi mentioned this pull request May 9, 2019
Closed
@eerhardt
Copy link
Member

Can we be sure to add some tests for this new feature when it is ready?

@codemzs
Copy link
Member

codemzs commented May 10, 2019

@eerhardt this is a work in progress PR. Let’s give the user the chance to remove the draft tag before we start reviewing. I’m working with Meng and she will add tests and even performance benchmarks.

@mengaims
Copy link
Contributor Author

mengaims commented May 12, 2019 via email

@artidoro
Copy link
Contributor

artidoro commented May 16, 2019 •
edited
Loading

Could you also point to some benchmarks/tests where you show that it performs well?
Also it would be great to have a description of the algorithm that you are implementing in the docs.

Either in this PR or in another we will also need to make sure we have samples for this trainer.

@mengaims
Copy link
Contributor Author

Could you also point to some benchmarks/tests where you show that it performs well?
Also it would be great to have a description of the algorithm that you are implementing in the docs.

Either in this PR or in another we will also need to make sure we have samples for this trainer.

Sure. I'm now working on the benchmark now. And the samples are provided already, I will add more descriptions and comments.

@mengaims mengaims marked this pull request as ready for review May 22, 2019 13:26
@codemzs codemzs requested review from eerhardt and ganik May 22, 2019 16:33
@eerhardt
Copy link
Member

// Licensed to the .NET Foundation under one or more agreements.

This file's name is inconsistent with the rest of the files: SRCNN vs. SrCnn

Refers to: src/Microsoft.ML.TimeSeries/SRCNNAnomalyDetector.cs:1 in 406c664. [](commit_id = 406c664, deletion_comment = False)

mengaims added 2 commits May 27, 2019 01:15
@mengaims
Merge remote-tracking branch 'upstream/master' into timeseries/sr
cc0b479
@mengaims
Fix commented problems:
c56d4d4 
1.Add xml to DetectAnomalyBySrCnn;
2.Add default value to DetectAnomalyBySrCnn;
3.Change class name of SrCnnAnomalyDetectionBaseWrapper;
4.fix AlertThreshold write bug;
5.Add check argument part and remove redundent TODOs
6.Fix slot name filling bug.
@mengaims mengaims changed the title Add SrCnn Anomaly Detector Fixes #3799: Add SrCnn Anomaly Detector May 31, 2019
@mengaims mengaims changed the title Fixes #3799: Add SrCnn Anomaly Detector fixes #3799: Add SrCnn Anomaly Detector May 31, 2019
codemzs
codemzs approved these changes May 31, 2019
View reviewed changes
Copy link
Member

@codemzs codemzs 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 but please address Wei-Sheng's comments. Thanks @mengaims !

wschin
wschin approved these changes May 31, 2019
View reviewed changes
Copy link
Member

@wschin wschin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The code framework looks good. Just two points for improvements needed in next PR.

  • Please add prediction equation in another PR. You can use Latex syntax.
  • Please try to avoid frequent float[] allocation in C# whenever possible. I saw some in State's function.

Thanks!

@codemzs codemzs changed the title fixes #3799: Add SrCnn Anomaly Detector Add SrCnn Anomaly Detector May 31, 2019
@codemzs codemzs closed this May 31, 2019
@codemzs codemzs reopened this May 31, 2019
@mengaims mengaims merged commit 048d828 into dotnet:master May 31, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Ivanidzo4ka Ivanidzo4ka Ivanidzo4ka left review comments

@eerhardt eerhardt eerhardt left review comments

@codemzs codemzs codemzs approved these changes

@wschin wschin wschin approved these changes

@ganik ganik Awaiting requested review from ganik

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add SrCnn Anomaly Detection algorithm
7 participants
@mengaims @dnfclas @eerhardt @codemzs @artidoro @Ivanidzo4ka @wschin