Skip to content
2 changes: 1 addition & 1 deletion src/Microsoft.ML.Api/DataViewConstructionUtils.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ namespace Microsoft.ML.Runtime.Api
/// <summary>
/// A helper class to create data views based on the user-provided types.
/// </summary>
internal static class DataViewConstructionUtils
public static class DataViewConstructionUtils

@TomFinley TomFinley May 9, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

public static class DataViewConstructionUtils [](start = 4, length = 45)

This makes me nervous... If you make this public rather than internal, then it is no longer appropriate to have what are currently Asserts on the public methods. They'll have to be Checks, with suitably verbose error messages, properly called in the context of env, and so on, and so on. This is more work than I think you want to have right now.

Do you need all of the functions in this class? This was originally a supporting utility class for the pre-ML.NET API. You don't need all of it... in fact I think you only need a couple of them. I might choose to either (1) introduce another public class with the methods you need being accessible through that or (2) change this class so that it remains public but everything in it has its access member changed to internal, except the one or two facilities you actually need. #Closed

@Ivanidzo4ka Ivanidzo4ka May 9, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh, I miss these blobs of text so much. (I'm serious right now) #Closed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Someone already did that for me. All I had to do is to look for method reference :)


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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks Ivan, it's nice to be appreciated. :D :D


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

{
public static IDataView CreateFromList<TRow>(IHostEnvironment env, IList<TRow> data,

@Ivanidzo4ka Ivanidzo4ka May 9, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

IList [](start = 75, length = 5)

Since we in this file, @tfinley@gmail.com do you have any opinion about IList?
I would personally convert it to List. #Resolved

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a reason why you would prefer List here?

So the reason I would prefer a specific implementer of an interface rather than the interface itself is to avoid having to go through the virtual function table. (Which is why it's often better to write Foo<TBar>(TBar biz) where TBar : IBar vs. Foo(IBar biz).) In this specific case though, I don't think it's worth it. You're ultimately going to be dealing with an IEnumerator<TRow> anyway for most operations on this structure, and there it's unfortunately unavoidable, so I care a bit less. But perhaps there's something to this I'm not appreciating.


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

@Ivanidzo4ka Ivanidzo4ka May 10, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I guess its just personal. I just don't like IList. In same time, IList allow you to pass Arrays, so this solve problem with Array support.


In reply to: 187207615 [](ancestors = 187207615,187205023)

SchemaDefinition schemaDefinition = null)
Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.ML.Api/InternalSchemaDefinition.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ namespace Microsoft.ML.Runtime.Api
/// <summary>
/// An internal class that holds the (already validated) mapping between a custom type and an IDataView schema.
/// </summary>
internal sealed class InternalSchemaDefinition
public sealed class InternalSchemaDefinition

@TomFinley TomFinley May 9, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

public sealed class InternalSchemaDefinition [](start = 4, length = 44)

I have similar concerns about this class being public. If we can avoid it, possibly by moving some of the logic current in MemoryCollection.cs to this assembly, then calling that from the Microsoft.ML project, I'd be delighted. #Closed

{
public readonly Column[] Columns;

Expand Down
28 changes: 28 additions & 0 deletions src/Microsoft.ML/CSharpApi.cs
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,22 @@ public Microsoft.ML.Data.TextLoader.Output Add(Microsoft.ML.Data.TextLoader inpu
return output;
}

public Microsoft.ML.Data.DataViewReference.Output Add(Microsoft.ML.Data.DataViewReference input)
{
var output = new Microsoft.ML.Data.DataViewReference.Output();
Add(input, output);
return output;
}

public void Add(Microsoft.ML.Data.TextLoader input, Microsoft.ML.Data.TextLoader.Output output)
{
_jsonNodes.Add(Serialize("Data.TextLoader", input, output));
}

public void Add(Microsoft.ML.Data.DataViewReference input, Microsoft.ML.Data.DataViewReference.Output output)
{
_jsonNodes.Add(Serialize("Data.DataViewReference", input, output));
}
public Microsoft.ML.Models.AnomalyDetectionEvaluator.Output Add(Microsoft.ML.Models.AnomalyDetectionEvaluator input)
{
var output = new Microsoft.ML.Models.AnomalyDetectionEvaluator.Output();
Expand Down Expand Up @@ -1311,6 +1322,23 @@ public sealed partial class TextLoader
public string CustomSchema { get; set; }


public sealed class Output
{
/// <summary>
/// The resulting data view
/// </summary>
public Var<Microsoft.ML.Runtime.Data.IDataView> Data { get; set; } = new Var<Microsoft.ML.Runtime.Data.IDataView>();

}
}

public sealed partial class DataViewReference
{
/// <summary>
/// Location of the input file
/// </summary>
public Var<Microsoft.ML.Runtime.Data.IDataView> Data { get; set; } = new Var<Microsoft.ML.Runtime.Data.IDataView>();

public sealed class Output
{
/// <summary>
Expand Down
66 changes: 66 additions & 0 deletions src/Microsoft.ML/MemoryCollection.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
using System;
using System.Collections.Generic;
using System.Text;
using Microsoft.ML.Runtime;
using Microsoft.ML.Runtime.Api;
using Microsoft.ML.Runtime.Data;
using Microsoft.ML.Runtime.EntryPoints;

namespace Microsoft.ML

@glebuk glebuk May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

.ML [](start = 19, length = 3)

ML.Data #Closed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

TextLoader is part of just ML, should I change it as well?


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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

good question... seems they both should be in data. the argument went about like this - can a user has out of the box experience with just ML namespace.
I guess we can keep it in ML for now.


In reply to: 187221799 [](ancestors = 187221799,187221578)

@codemzs codemzs May 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please move to Data. My PR will move TextLoader to ML.Data. #Resolved

{
public class MemoryCollection<TInput> : ILearningPipelineLoader
where TInput : class
{
public ILearningPipelineStep ApplyStep(ILearningPipelineStep previousStep, Experiment experiment)
{
Contracts.Assert(previousStep == null);
_dataViewEntryPoint = new Data.DataViewReference();
var importOutput = experiment.Add(_dataViewEntryPoint);
return new MemoryCollectionPipelineStep(importOutput.Data);
}

private readonly IList<TInput> _listCollection;
private readonly IEnumerable<TInput> _enumerableCollection;

private Data.DataViewReference _dataViewEntryPoint;
private IDataView _dataView;

public MemoryCollection(IList<TInput> collection)

@Ivanidzo4ka Ivanidzo4ka May 9, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

MemoryCollection [](start = 15, length = 16)

public constructor required comments. #Resolved

{
//need validation at some point

@TomFinley TomFinley May 9, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

//need validation at some point [](start = 12, length = 31)

How about we at least validate to ensure that these aren't null, with a Contracts.CheckParamValue? Here and in the other constructor. #Closed

_listCollection = collection;
}

public MemoryCollection(IEnumerable<TInput> collection)
{
//need validation at some point
_enumerableCollection = collection;
}

public void SetInput(IHostEnvironment env, Experiment experiment)
{
if (_listCollection!=null)
{

@TomFinley TomFinley May 9, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ctrl-k-d, also, maybe since these are one-liners we can omit these vebose {s. #Closed

_dataView = DataViewConstructionUtils.CreateFromList(env, _listCollection);
}
if (_enumerableCollection!=null)
{
_dataView = DataViewConstructionUtils.CreateFromEnumerable(env, _listCollection);
}
env.CheckValue(_dataView, nameof(_dataView));
experiment.SetInput(_dataViewEntryPoint.Data, _dataView);
}

private class MemoryCollectionPipelineStep : ILearningPipelineDataStep
{
public MemoryCollectionPipelineStep(Var<IDataView> data)
{
Data = data;
Model = null;

@TomFinley TomFinley May 9, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unnecessary, the default value is null.

That's fine, but I'd go one step further... What I'd do is change the public Var<ITransformModel> Model { get; } below to public Var<ITransformModel> Model => null;, that way you avoid having any backing field whatsoever. (Plus you save three characters, which totally makes it worth it. :D ) #Closed

}

public Var<IDataView> Data { get; }
public Var<ITransformModel> Model { get; }
}
}
}
34 changes: 34 additions & 0 deletions src/Microsoft.ML/Runtime/EntryPoints/DataViewReference.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
using Microsoft.ML.Data;
using Microsoft.ML.Runtime;
using Microsoft.ML.Runtime.CommandLine;
using Microsoft.ML.Runtime.Data;
using Microsoft.ML.Runtime.EntryPoints;

[assembly: LoadableClass(typeof(void), typeof(InMemoryDataView), null, typeof(SignatureEntryPointModule), "InMemoryDataView")]
namespace Microsoft.ML.Runtime.EntryPoints
{
public class InMemoryDataView
{
public sealed class Input
{
[Argument(ArgumentType.Required, ShortName = "data", HelpText = "Pointer to IDataView in memory", SortOrder = 1)]
public IDataView Data;
}

public sealed class Output
{
[TlcModule.Output(Desc = "The resulting data view", SortOrder = 1)]
public IDataView Data;
}

[TlcModule.EntryPoint(Name = "Data.DataViewReference", Desc = "Pass dataview from memory to experiment")]
public static Output ImportData(IHostEnvironment env, Input input)
{
Contracts.CheckValue(env, nameof(env));
var host = env.Register("DataViewReference");
env.CheckValue(input, nameof(input));
EntryPointUtils.CheckInputArgs(host, input);
return new Output { Data = input.Data };
}
}
}
176 changes: 176 additions & 0 deletions test/Microsoft.ML.Tests/MemoryCollectionTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,176 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.ML;
using Microsoft.ML.Runtime;
using Microsoft.ML.Runtime.Api;
using Microsoft.ML.Runtime.Data;
using Microsoft.ML.TestFramework;
using Microsoft.ML.Trainers;
using Microsoft.ML.Transforms;
using System;
using System.Collections.Generic;
using Xunit;
using Xunit.Abstractions;

namespace Microsoft.ML.EntryPoints.Tests
{
public class MemoryCollectionTests : BaseTestClass
{
public MemoryCollectionTests(ITestOutputHelper output)
: base(output)
{

}

[Fact]
public void ConstructorDoesntThrow()
{
Assert.NotNull(new MemoryCollection<Input>(new List<Input>()));
Assert.NotNull(new MemoryCollection<Input>(new List<Input>() { new Input { Number1 = 1, String1 = "1" } }));
Assert.NotNull(new MemoryCollection<Input>(new Input[0]));
Assert.NotNull(new MemoryCollection<Input>(new Input[1] { new Input { Number1 = 1, String1 = "1" } }));
Assert.NotNull(new MemoryCollection<Input>(null));
}

[Fact]
public void CanSuccessfullyApplyATransform()
{
var collection = new MemoryCollection<Input>(new List<Input>() { new Input { Number1 = 1, String1 = "1" } });

@TomFinley TomFinley May 9, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

MemoryCollection [](start = 33, length = 23)

Just an observation... if we had instead structured this as a static utility method somewhere, then we could avoid having the double-specification of the Input class, as it would have been inferred by the compiler.

So: if we had a input type MyAwesomeInputType, then instead of MemoryCollection<MyAwesomeInputType>(new MyAwesomeInputType[] {...}), we would have MemoryCollection.Create(new MyAwesomeInputType[] {...}) since the compiler could have done the work of inferring the type and whatnot. #Closed


using (var environment = new TlcEnvironment())
{
Experiment experiment = environment.CreateExperiment();
ILearningPipelineDataStep output = collection.ApplyStep(null, experiment) as ILearningPipelineDataStep;

Assert.NotNull(output.Data);

@TomFinley TomFinley May 9, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Assert.NotNull(output.Data); [](start = 16, length = 28)

If you really meant the as above, you should first Assert.NonNull on output. If you didn't mean the as above, then you should do a direct () style cast to ILearningPipelineDataStep. The only reason to use an as style cast is if you are entertaining the possibility that it might not implement that interface, in which case, given that this is a test, you should test that. (And if it wasn't a test, you would Contracts.Check* it.) #Closed

Assert.NotNull(output.Data.VarName);
Assert.Null(output.Model);
}
}

[Fact]
public void CanSuccessfullyEnumerated()
{
var collection = new MemoryCollection<Input>(new List<Input>() {
new Input { Number1 = 1, String1 = "1" },
new Input { Number1 = 2, String1 = "2" },
new Input { Number1 = 3, String1 = "3" }
});

using (var environment = new TlcEnvironment())
{
Experiment experiment = environment.CreateExperiment();
ILearningPipelineDataStep output = collection.ApplyStep(null, experiment) as ILearningPipelineDataStep;

experiment.Compile();
collection.SetInput(environment, experiment);
experiment.Run();

IDataView data = experiment.GetOutput(output.Data);
Assert.NotNull(data);

using (var cursor = data.GetRowCursor((a => true)))
{
var IDGetter = cursor.GetGetter<float>(0);
var TextGetter = cursor.GetGetter<DvText>(1);

Assert.True(cursor.MoveNext());

float ID = 0;
IDGetter(ref ID);
Assert.Equal(1, ID);

DvText Text = new DvText();
TextGetter(ref Text);
Assert.Equal("1", Text.ToString());

Assert.True(cursor.MoveNext());

ID = 0;
IDGetter(ref ID);
Assert.Equal(2, ID);

Text = new DvText();
TextGetter(ref Text);
Assert.Equal("2", Text.ToString());

Assert.True(cursor.MoveNext());

ID = 0;
IDGetter(ref ID);
Assert.Equal(3, ID);

Text = new DvText();
TextGetter(ref Text);
Assert.Equal("3", Text.ToString());

Assert.False(cursor.MoveNext());
}
}
}

[Fact]
public void CanTrain()
{
var pipeline = new LearningPipeline();
var collection = new MemoryCollection<IrisData>(new List<IrisData>() {
new IrisData { SepalLength = 1f, SepalWidth = 1f ,PetalLength=0.3f, PetalWidth=5.1f, Label=1},
new IrisData { SepalLength = 1f, SepalWidth = 1f ,PetalLength=0.3f, PetalWidth=5.1f, Label=1},
new IrisData { SepalLength = 1.2f, SepalWidth = 0.5f ,PetalLength=0.3f, PetalWidth=5.1f, Label=0}
});

pipeline.Add(collection);

pipeline.Add(new ColumnConcatenator(outputColumn: "Features",
"SepalLength", "SepalWidth", "PetalLength", "PetalWidth"));

pipeline.Add(new StochasticDualCoordinateAscentClassifier());
PredictionModel<IrisData, IrisPrediction> model = pipeline.Train<IrisData, IrisPrediction>();

IrisPrediction prediction = model.Predict(new IrisData()
{
SepalLength = 3.3f,
SepalWidth = 1.6f,
PetalLength = 0.2f,
PetalWidth = 5.1f,
});

}

public class Input
{
[Column("0")]
public float Number1;

[Column("1")]
public string String1;
}

public class IrisData
{
[Column("0")]
public float Label;

[Column("1")]
public float SepalLength;

[Column("2")]
public float SepalWidth;

[Column("3")]
public float PetalLength;

[Column("4")]
public float PetalWidth;
}

public class IrisPrediction
{
[ColumnName("Score")]
public float[] PredictedLabels;
}

}
}