Prediction engine for time series. #1618

codemzs · 2018-11-14T17:58:18Z

Prediction engine for time series that updates the time series transform state at the time of prediction. Next iteration will add more tests.

src/Microsoft.ML.Core/Data/MetadataUtils.cs

src/Microsoft.ML.Data/DataView/CompositeRowToRowMapper.cs

src/Microsoft.ML.TimeSeries/PredictionEngine.cs

src/Microsoft.ML.TimeSeries/SequentialTransformerBase.cs

src/Microsoft.ML.Api/InternalSchemaDefinition.cs

test/Microsoft.ML.TimeSeries.Tests/TimeSeriesDirectApi.cs

src/Microsoft.ML.TimeSeries/PredictionEngine.cs

Zruty0 · 2018-11-14T22:19:22Z

src/Microsoft.ML.TimeSeries/PredictionEngine.cs

+
+            if (rows.Count == 0 && outputRow is IStatefulRow)
+                rows.Add((IStatefulRow)outputRow);
+


this feels like an unroll of 1 level of recursion. Can't GetStatefulRows also handle case where mapper is not a CompositeRowToRowMapper? #Resolved

still active

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

src/Microsoft.ML.TimeSeries/PredictionEngine.cs

src/Microsoft.ML.TimeSeries/SequentialTransformerBase.cs

Zruty0

🕐

…to timeseriesengine

src/Microsoft.ML.Api/PredictionEngine.cs

src/Microsoft.ML.TimeSeries/PredictionEngine.cs

Zruty0 · 2018-11-21T22:20:02Z

src/Microsoft.ML.TimeSeries/PredictionEngine.cs

+            if (prediction == null)
+                prediction = new TDst();
+
+            //Update State.


//Up [](start = 12, length = 4)

// Update state. and // Predict. #Resolved

Zruty0 · 2018-11-21T22:21:27Z

src/Microsoft.ML.TimeSeries/SequentialTransformerBase.cs

+    /// to get the dependencies on input columns and the getters for the output columns, given an active set of output columns.
+    /// </summary>
+
+    public sealed class TimeSeriesRowToRowMapperTransform : RowToRowTransformBase, IRowToRowMapper,


TimeSeriesRowToRowMapperTransform [](start = 24, length = 33)

Why did you need to clone it, and why is it public? #Pending

i'll refactor this out and make it internal

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

src/Microsoft.ML.TimeSeries/SequentialTransformerBase.cs

…to timeseriesengine

Zruty0 · 2018-11-23T18:55:12Z

src/Microsoft.ML.Api/PredictionEngine.cs

        /// <summary>
        /// Run prediction pipeline on one example.
        /// </summary>
        /// <param name="example">The example to run on.</param>
        /// <param name="prediction">The object to store the prediction in. If it's <c>null</c>, a new one will be created, otherwise the old one
        /// is reused.</param>
-        public void Predict(TSrc example, ref TDst prediction)
+        public virtual void Predict(TSrc example, ref TDst prediction)


virtual [](start = 15, length = 7)

If 2 existing implementations are dissimilar, what's the value in having a virtual method? Make it abstract #Resolved

Zruty0 · 2018-11-23T18:57:55Z

src/Microsoft.ML.Api/PredictionFunction.cs

@@ -7,30 +7,40 @@

 namespace Microsoft.ML.Runtime.Data
 {
+    public sealed class PredictionFunction<TSrc, TDst> : PredictionFunctionBase<TSrc, TDst>


PredictionFunction [](start = 24, length = 18)

I honestly don't understand the purpose of PredictionFunction. Why introduce a hierarchy of useless classes? I would much rather rename PredictionEngine to PredictionFunction and be done with it.

It doesn't have to be done in this PR, but could you just use PredictionEngine only, to simplify that future change? #Resolved

I'm with you 100% on this. I have the same thought when I was making this change.

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

Zruty0 · 2018-11-23T18:58:31Z

src/Microsoft.ML.Api/PredictionFunction.cs

        }

+        internal virtual void CreatePredictionEngine(IHostEnvironment env, ITransformer transformer, out PredictionEngineBase<TSrc, TDst> engine) =>


CreatePredictionEngine [](start = 30, length = 22)

use return value instead of out parameter #Resolved

Zruty0 · 2018-11-23T19:02:19Z

src/Microsoft.ML.Data/DataLoadSave/TransformerChain.cs

    /// <summary>
    /// A chain of transformers (possibly empty) that end with a <typeparamref name="TLastTransformer"/>.
    /// For an empty chain, <typeparamref name="TLastTransformer"/> is always <see cref="ITransformer"/>.
    /// </summary>
-    public sealed class TransformerChain<TLastTransformer> : ITransformer, ICanSaveModel, IEnumerable<ITransformer>
+    public sealed class TransformerChain<TLastTransformer> : ITransformer, ICanSaveModel, IEnumerable<ITransformer>, ITransformerAccessor
    where TLastTransformer : class, ITransformer


This is confusing. You now can access the inner transformers in 3 ways: via foreach (as an IEnumerable), via the internal field Transformers and via ITransformerAccessor.Transformers. Let's reduce it to one. Maybe make in an IEnumerable of pairs, if you need access to scopes #Resolved

Or you could make Transformers and Scopes public and IReadOnlyLists

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

I created this interface so that I can access transformers and scope from an ITransfomer reference because casting ITransfomer to TransformerChain is messy.

In reply to: 236007746 [](ancestors = 236007746,236007659)

Zruty0 · 2018-11-23T19:05:36Z

src/Microsoft.ML.TimeSeries/SequentialAnomalyDetectionTransformBase.cs

@@ -306,10 +307,10 @@ public abstract class AnomalyDetectionStateBase : StateBase
            protected SequentialAnomalyDetectionTransformBase<TInput, TState> Parent;

            // A windowed buffer to cache the update values to the martingale score in the log scale.
-            private FixedSizeQueue<Double> _logMartingaleUpdateBuffer;
+            private FixedSizeQueue<Double> LogMartingaleUpdateBuffer { get; set; }


LogMartingaleUpdateBuffer [](start = 43, length = 25)

why this change? #Resolved

also, could you add this assembly to be tracked by RoboTom?

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

We need this value for saving the state in the case of IID. I will add TS binary to be tracked by RoboTom.

In reply to: 236008263 [](ancestors = 236008263,236008010)

Zruty0 · 2018-11-23T19:07:57Z

using Microsoft.ML.Core.Data;

summary comment #Resolved

Refers to: src/Microsoft.ML.TimeSeries/PredictionEngine.cs:1 in 7bf4c39. [](commit_id = 7bf4c39, deletion_comment = False)

Zruty0 · 2018-11-23T19:13:01Z

src/Microsoft.ML.TimeSeries/SequentialTransformerBase.cs

-        public bool IsRowToRowMapper => false;
+        public bool IsRowToRowMapper => true;
+
+        public TState StateRef{ get; set; }


public TState StateRef{ get; set; } [](start = 8, length = 35)

I don't see this as a good idea at all.
The whole cloning mechanism could be implemented completely internally, without making anything public, or mutable. Just don't use MemberwiseClone, and write it yourself. This also has the benefit of self-documenting what's being cloned and how. #Pending

Sounds good.

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

Zruty0 · 2018-11-23T19:13:48Z

you'll need to revert your changes to libmf. #Resolved

Zruty0 · 2018-11-23T19:22:14Z

src/Microsoft.ML.TimeSeries/PredictionEngine.cs

+    {
+        private ValuePinger[][] _pingers;
+        private int _rowPosition;
+        public ITransformer CheckPoint => Transformer;


Transformer [](start = 42, length = 11)

This would mean that if you checkpoint something, it's a mutable object. As far as I recall, we agreed to keep mutable transformers completely inside the prediction engine. This means that you need to CloneTransformers here.

Also, make it a method: ITransformer GetCurrentTransformer() or something like this. #Pending

I'm confused but please help me understand your concern here. Isn't that what I'm doing below? I have changed the API for checkpointing where the user passes a modelpath and prediction engine writes the model with updated state there.

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

Zruty0 · 2018-11-23T19:26:32Z

I finished reviewing the iteration 24 on 11/23. This code still has minor cleanup needed, but I'm more concerned of whether the batch prediction is affected (for some reason you had to skip the time series tests). #Resolved

revoking review

codemzs · 2018-11-26T06:52:31Z

Thanks, Pete. Batch Prediction is not affected, I have enabled batch prediction tests and they are passing.

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

…to timeseriesengine

prediction engine for time series.

a87d836

codemzs requested a review from ganik November 14, 2018 17:58

codemzs added 2 commits November 14, 2018 10:12

clean up.

227adb6

clean up.

7a7fe64

TomFinley reviewed Nov 14, 2018

View reviewed changes

src/Microsoft.ML.Core/Data/MetadataUtils.cs Outdated Show resolved Hide resolved

TomFinley reviewed Nov 14, 2018

View reviewed changes

src/Microsoft.ML.Data/DataView/CompositeRowToRowMapper.cs Outdated Show resolved Hide resolved

TomFinley reviewed Nov 14, 2018

View reviewed changes

src/Microsoft.ML.TimeSeries/PredictionEngine.cs Outdated Show resolved Hide resolved

TomFinley reviewed Nov 14, 2018

View reviewed changes

src/Microsoft.ML.TimeSeries/SequentialTransformerBase.cs Outdated Show resolved Hide resolved

TomFinley reviewed Nov 14, 2018

View reviewed changes

src/Microsoft.ML.Api/InternalSchemaDefinition.cs Outdated Show resolved Hide resolved

Zruty0 reviewed Nov 14, 2018

View reviewed changes

test/Microsoft.ML.TimeSeries.Tests/TimeSeriesDirectApi.cs Outdated Show resolved Hide resolved

Zruty0 reviewed Nov 14, 2018

View reviewed changes

src/Microsoft.ML.TimeSeries/PredictionEngine.cs Outdated Show resolved Hide resolved

Zruty0 reviewed Nov 14, 2018

View reviewed changes

src/Microsoft.ML.TimeSeries/PredictionEngine.cs Outdated Show resolved Hide resolved

Zruty0 reviewed Nov 14, 2018

View reviewed changes

src/Microsoft.ML.TimeSeries/SequentialTransformerBase.cs Outdated Show resolved Hide resolved

Zruty0 previously requested changes Nov 14, 2018

View reviewed changes

codemzs added 11 commits November 14, 2018 16:39

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

8fc8307

…to timeseriesengine

refactor code.

cae6f89

refactor code.

8381205

refactor code.

ccaabd4

refactor code.

11b42e4

refactor code.

7d21d78

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

878e87d

…to timeseriesengine

refactor code.

745ff1c

PR feedback.

6adadee

PR feedback.

31b91e7

PR feedback.

75ec0b7

TomFinley reviewed Nov 16, 2018

View reviewed changes

src/Microsoft.ML.Api/PredictionEngine.cs Outdated Show resolved Hide resolved

TomFinley reviewed Nov 16, 2018

View reviewed changes

src/Microsoft.ML.TimeSeries/PredictionEngine.cs Outdated Show resolved Hide resolved

Ivanidzo4ka mentioned this pull request Nov 19, 2018

WIP stateful prediction engine for TS. #1526

Closed

checkpoint.

0efc46d

Zruty0 reviewed Nov 21, 2018

View reviewed changes

src/Microsoft.ML.TimeSeries/SequentialTransformerBase.cs Outdated Show resolved Hide resolved

codemzs added 3 commits November 23, 2018 02:38

misc fixes.

e8d10e5

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

54593d3

…to timeseriesengine

misc fixes.

7bf4c39

Zruty0 reviewed Nov 23, 2018

View reviewed changes

codemzs added 3 commits November 25, 2018 22:53

PR feedback.

dbf3a20

cleanup.

a6e6d27

revert libmf.

ffcc045

codemzs requested a review from montebhoover November 26, 2018 18:07

codemzs added 2 commits November 26, 2018 14:26

PR feedback.

a28d461

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

3247537

…to timeseriesengine

TomFinley mentioned this pull request Nov 26, 2018

Need PredictionFunction to provide object-pooling or to be thread-safe #1718

Closed

codemzs added 2 commits November 26, 2018 15:04

PR feedback.

4c27aa1

PR feedback.

1a660e4

codemzs mentioned this pull request Nov 26, 2018

Stateful Prediction engine for time series. #1727

Merged

codemzs closed this Nov 27, 2018

ghost locked as resolved and limited conversation to collaborators Mar 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prediction engine for time series. #1618

Prediction engine for time series. #1618

codemzs commented Nov 14, 2018

Zruty0 Nov 14, 2018 •

edited by codemzs

Loading

Zruty0 Nov 21, 2018

Zruty0 left a comment

Zruty0 Nov 21, 2018 •

edited by codemzs

Loading

Zruty0 Nov 21, 2018 •

edited by codemzs

Loading

codemzs Nov 26, 2018

Zruty0 Nov 23, 2018 •

edited by codemzs

Loading

Zruty0 Nov 23, 2018 •

edited by codemzs

Loading

codemzs Nov 26, 2018

Zruty0 Nov 23, 2018 •

edited by codemzs

Loading

Zruty0 Nov 23, 2018 •

edited by codemzs

Loading

Zruty0 Nov 23, 2018

codemzs Nov 26, 2018

Zruty0 Nov 23, 2018 •

edited by codemzs

Loading

Zruty0 Nov 23, 2018

codemzs Nov 26, 2018

Zruty0 commented Nov 23, 2018 •

edited by codemzs

Loading

Zruty0 Nov 23, 2018 •

edited by codemzs

Loading

codemzs Nov 26, 2018

Zruty0 commented Nov 23, 2018 •

edited by codemzs

Loading

Zruty0 Nov 23, 2018 •

edited by codemzs

Loading

codemzs Nov 26, 2018

Zruty0 commented Nov 23, 2018 •

edited by codemzs

Loading

codemzs commented Nov 26, 2018 •

edited

Loading


		if (rows.Count == 0 && outputRow is IStatefulRow)
		rows.Add((IStatefulRow)outputRow);

		}

		internal virtual void CreatePredictionEngine(IHostEnvironment env, ITransformer transformer, out PredictionEngineBase<TSrc, TDst> engine) =>

Prediction engine for time series. #1618

Prediction engine for time series. #1618

Conversation

codemzs commented Nov 14, 2018

Zruty0 Nov 14, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 left a comment

Choose a reason for hiding this comment

Zruty0 Nov 21, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

Zruty0 Nov 21, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Nov 23, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

Zruty0 Nov 23, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Nov 23, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

Zruty0 Nov 23, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Nov 23, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 commented Nov 23, 2018 • edited by codemzs Loading

Zruty0 Nov 23, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 commented Nov 23, 2018 • edited by codemzs Loading

Zruty0 Nov 23, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 commented Nov 23, 2018 • edited by codemzs Loading

codemzs commented Nov 26, 2018 • edited Loading

Zruty0 Nov 14, 2018 •

edited by codemzs

Loading

Zruty0 Nov 21, 2018 •

edited by codemzs

Loading

Zruty0 Nov 21, 2018 •

edited by codemzs

Loading

Zruty0 Nov 23, 2018 •

edited by codemzs

Loading

Zruty0 Nov 23, 2018 •

edited by codemzs

Loading

Zruty0 Nov 23, 2018 •

edited by codemzs

Loading

Zruty0 Nov 23, 2018 •

edited by codemzs

Loading

Zruty0 Nov 23, 2018 •

edited by codemzs

Loading

Zruty0 commented Nov 23, 2018 •

edited by codemzs

Loading

Zruty0 Nov 23, 2018 •

edited by codemzs

Loading

Zruty0 commented Nov 23, 2018 •

edited by codemzs

Loading

Zruty0 Nov 23, 2018 •

edited by codemzs

Loading

Zruty0 commented Nov 23, 2018 •

edited by codemzs

Loading

codemzs commented Nov 26, 2018 •

edited

Loading