Skip to content

Add Seasonality Detection #5200

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

Closed
wants to merge 5 commits into from
Closed

Conversation

guinao
Copy link
Contributor

@guinao guinao commented Jun 3, 2020

We are excited to review your PR.

So we can do the best job, please check:

  • There's a descriptive title that will make sense to other developers some time from now.
  • There's associated issues. All PR's should have issue(s) associated - unless a trivial self-evident change such as fixing a typo. You can use the format Fixes #nnnn in your description to cause GitHub to automatically close the issue(s) when your PR is merged.
  • Your change description explains what the change does, why you chose your approach, and anything else that reviewers should know.
  • You have included any necessary tests in the same PR.

@guinao guinao requested a review from a team as a code owner June 3, 2020 08:21
@dnfadmin
Copy link

dnfadmin commented Jun 3, 2020

CLA assistant check
All CLA requirements met.

@guinao guinao marked this pull request as draft June 3, 2020 08:25
@guinao guinao marked this pull request as ready for review June 3, 2020 08:26
@codecov
Copy link

codecov bot commented Jun 3, 2020

Codecov Report

Merging #5200 into master will decrease coverage by 2.56%.
The diff coverage is 12.54%.

@@            Coverage Diff             @@
##           master    #5200      +/-   ##
==========================================
- Coverage   75.78%   73.22%   -2.57%     
==========================================
  Files         993     1009      +16     
  Lines      180969   188212    +7243     
  Branches    19489    20282     +793     
==========================================
+ Hits       137151   137812     +661     
- Misses      38521    44873    +6352     
- Partials     5297     5527     +230     
Flag Coverage Δ
#Debug 73.22% <12.54%> (-2.57%) ⬇️
#production 68.95% <9.38%> (-2.76%) ⬇️
#test 87.44% <100.00%> (-1.45%) ⬇️
Impacted Files Coverage Δ
src/Microsoft.ML.TimeSeries/MathUtils.cs 0.00% <0.00%> (ø)
src/Microsoft.ML.TimeSeries/PeriodDetectUtils.cs 0.00% <0.00%> (ø)
...rc/Microsoft.ML.TimeSeries/SRCNNAnomalyDetector.cs 30.90% <ø> (ø)
...crosoft.ML.TimeSeries/RootCauseLocalizationType.cs 51.19% <70.00%> (ø)
src/Microsoft.ML.TimeSeries/RootCauseAnalyzer.cs 56.44% <78.26%> (ø)
src/Microsoft.ML.TimeSeries/ExtensionsCatalog.cs 95.00% <100.00%> (ø)
...crosoft.ML.TimeSeries.Tests/TimeSeriesDirectApi.cs 99.53% <100.00%> (ø)
...ML.Tests/Scenarios/IrisPlantClassificationTests.cs 0.00% <0.00%> (-100.00%) ⬇️
test/Microsoft.ML.AutoML.Tests/Util.cs 50.00% <0.00%> (-50.00%) ⬇️
...st/Microsoft.ML.Functional.Tests/Datasets/Adult.cs 58.82% <0.00%> (-41.18%) ⬇️
... and 222 more

@guinao guinao changed the title Dev/seasonality Add Seasonality Detection Jun 3, 2020
using System.Text;

namespace Microsoft.ML.TimeSeries
{
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR doesn't integrate with any other code in ML.NET. How does this implement seasonality detection?
Can you please update it or close it if this is not going to be developed further?

Copy link
Contributor

Choose a reason for hiding this comment

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

I will update it NLT tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please review PR 5231 and I have asked the PR owner to abandon this PR.

@lisahua
Copy link
Contributor

lisahua commented Jun 11, 2020

Can we abandon this PR and replace it with PR 5231?

@harishsk
Copy link
Contributor

Closing because this has been superseded by #5231

@harishsk harishsk closed this Jun 12, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Mar 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants