Skip to content

Time Series samples for stateful prediction engine. #3213

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

Merged
merged 7 commits into from
Apr 11, 2019

Conversation

codemzs
Copy link
Member

@codemzs codemzs commented Apr 5, 2019

fixes #3277

Seems like PR #2900 removed the stateful prediction samples as part of refactoring and cleanup. Adding these useful samples back as they are used by customers that want to save time series model to disk and reload the model with up to date state that contains last seen values from prediction phase.

@codemzs codemzs requested review from shmoradims and sfilipi April 5, 2019 09:05
@codecov
Copy link

codecov bot commented Apr 5, 2019

Codecov Report

Merging #3213 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3213   +/-   ##
=======================================
  Coverage   72.62%   72.62%           
=======================================
  Files         807      807           
  Lines      145080   145080           
  Branches    16213    16213           
=======================================
  Hits       105369   105369           
- Misses      35294    35295    +1     
+ Partials     4417     4416    -1
Flag Coverage Δ
#Debug 72.62% <ø> (ø) ⬆️
#production 68.17% <ø> (ø) ⬆️
#test 88.92% <ø> (-0.01%) ⬇️
Impacted Files Coverage Δ
src/Microsoft.ML.TimeSeries/ExtensionsCatalog.cs 93.33% <ø> (ø) ⬆️
src/Microsoft.ML.TimeSeries/PredictionFunction.cs 87.2% <ø> (ø) ⬆️
...soft.ML.TestFramework/DataPipe/TestDataPipeBase.cs 73.7% <0%> (-0.34%) ⬇️
...ansforms/PermutationFeatureImportanceExtensions.cs 97.93% <0%> (ø) ⬆️
src/Microsoft.ML.Maml/MAML.cs 26.21% <0%> (+1.45%) ⬆️

@codemzs codemzs requested a review from sfilipi April 5, 2019 20:48
@sfilipi
Copy link
Member

sfilipi commented Apr 5, 2019

    /// [!code-csharp[MF](~/../docs/samples/docs/samples/Microsoft.ML.Samples/Dynamic/IidChangePointDetectorTransform.cs)]

did you want to update those to the new files?

Also, you can write in the CDATA, so that the examples have some sort of explanation on them.


Refers to: src/Microsoft.ML.TimeSeries/PredictionFunction.cs:272 in cab1b81. [](commit_id = cab1b81, deletion_comment = False)

@shmoradims
Copy link

shmoradims commented Apr 9, 2019

    class IidChangePointData

suggestion: let's call the data in all timeseries samples TimeSeriesData. There's nothing in the data that makes it IidChangePointData. In fact the same data can be used with all of the timeseries transforms. #Resolved


Refers to: docs/samples/Microsoft.ML.Samples/Dynamic/Transforms/TimeSeries/DetectIidChangePoint.cs:21 in 3aea63a. [](commit_id = 3aea63a, deletion_comment = False)

@shmoradims
Copy link

shmoradims commented Apr 9, 2019

            data.Add(new IidChangePointData(7));

suggestion: can we make these inline like this? it won't need preview and it's easier to see the data.


#Resolved


Refers to: docs/samples/Microsoft.ML.Samples/Dynamic/Transforms/TimeSeries/DetectIidChangePoint.cs:46 in 3aea63a. [](commit_id = 3aea63a, deletion_comment = False)

@codemzs
Copy link
Member Author

codemzs commented Apr 10, 2019

    /// [!code-csharp[MF](~/../docs/samples/docs/samples/Microsoft.ML.Samples/Dynamic/IidChangePointDetectorTransform.cs)]

I updated the file paths. Don't see any reason to write in CDATA because this is how we are doing it everywhere else. Lets be consistent and not waste time on small details.


In reply to: 480445339 [](ancestors = 480445339)


Refers to: src/Microsoft.ML.TimeSeries/PredictionFunction.cs:272 in 3aea63a. [](commit_id = 3aea63a, deletion_comment = False)

@codemzs codemzs force-pushed the timeseriessamples branch from 3aea63a to cab1b81 Compare April 10, 2019 20:49
Copy link

@shmoradims shmoradims left a comment

Choose a reason for hiding this comment

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

:shipit:

@sfilipi
Copy link
Member

sfilipi commented Apr 10, 2019

    /// [!code-csharp[MF](~/../docs/samples/docs/samples/Microsoft.ML.Samples/Dynamic/IidChangePointDetectorTransform.cs)]

I mean, in here, just a sentence in between the examples.

/// This is an example on how to identify changing points in the time series, accounting for seasonality.
/// [!code-csharpMF]
///
/// This is another example on how to identify spiking points in the time series, accounting for seasonality of data.
///
/// [!code-csharpMF]


In reply to: 481765669 [](ancestors = 481765669,480445339)


Refers to: src/Microsoft.ML.TimeSeries/PredictionFunction.cs:272 in cab1b81. [](commit_id = cab1b81, deletion_comment = False)

@codemzs
Copy link
Member Author

codemzs commented Apr 10, 2019

    /// [!code-csharp[MF](~/../docs/samples/docs/samples/Microsoft.ML.Samples/Dynamic/IidChangePointDetectorTransform.cs)]

I understand, however this pattern does not seem to be used elsewhere in the codebase. I look around and I see just links. Like I said, lets not waste time on details and just be consistent and move fast.


In reply to: 481867193 [](ancestors = 481867193,481765669,480445339)


Refers to: src/Microsoft.ML.TimeSeries/PredictionFunction.cs:272 in cab1b81. [](commit_id = cab1b81, deletion_comment = False)

@sfilipi
Copy link
Member

sfilipi commented Apr 10, 2019

    /// [!code-csharp[MF](~/../docs/samples/docs/samples/Microsoft.ML.Samples/Dynamic/IidChangePointDetectorTransform.cs)]

I'll add it in another PR ;)


In reply to: 481867611 [](ancestors = 481867611,481867193,481765669,480445339)


Refers to: src/Microsoft.ML.TimeSeries/PredictionFunction.cs:272 in cab1b81. [](commit_id = cab1b81, deletion_comment = False)

Copy link
Member

@sfilipi sfilipi left a comment

Choose a reason for hiding this comment

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

:shipit:

@codemzs codemzs merged commit ef0302a into dotnet:master Apr 11, 2019
codemzs added a commit to codemzs/machinelearning that referenced this pull request Apr 11, 2019
* Add time series samples for stateful prediction engine.

* PR feedback.

* PR feedback.

* PR feedback.

* cleanup.

* PR feedback.

* cleanup.
@ghost ghost locked as resolved and limited conversation to collaborators Mar 23, 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.

Samples for time series prediction engine and general clean up of all time series samples
3 participants