-
Notifications
You must be signed in to change notification settings - Fork 1.9k
CollectionDataSource (train on top of memory collection instead of loading data from file) #106
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
Changes from 8 commits
f69d659
55b6e46
b166f05
a1761b1
12d1b9e
ebcf448
110e205
62ab575
1da42ca
0cac7dc
ca9c031
d78afa3
ab86b09
ebe6f33
04ff469
9698d19
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| // 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 System.Collections.Generic; | ||
| using Microsoft.ML.Runtime; | ||
| using Microsoft.ML.Runtime.Api; | ||
| using Microsoft.ML.Runtime.Data; | ||
| using Microsoft.ML.Runtime.EntryPoints; | ||
| using Microsoft.ML.Runtime.Internal.Utilities; | ||
|
|
||
| namespace Microsoft.ML | ||
| { | ||
| public class MemoryCollection | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is this a static class? Why does it exist at all? Seems like a very strange name for a class #Closed
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a helper class, see comment from Tom starting from "Just an observation". In reply to: 187222326 [](ancestors = 187222326)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't you have that static method on the MemoryCollectionLoader? If it must be a different class vs the MemoryCollectionLoader then can you define a MemoryCollectionLoader with no generic type arg? In reply to: 187222607 [](ancestors = 187222607,187222326)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MemoryCollectionLoader is a generic class. Whole point to not type class twice. MemoryCollectionLoader.Create(Listdata) and MemoryCollection.Create(Listdata). In reply to: 187223070 [](ancestors = 187223070,187222607,187222326)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need the create method anyway? What's wrong with a good old constructor? Text loader does not have one. In reply to: 187223445 [](ancestors = 187223445,187223070,187222607,187222326)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For problem with "good old constructor," see earlier comment from the aforementioned Tom on "Just an observation." The more we can rely on the compiler to do the work for the user, the better, I'd say. Especially since this is merely a wrapper anyway, I don't see the point of making the user's job any harder than it has to be. In reply to: 187226013 [](ancestors = 187226013,187223445,187223070,187222607,187222326)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we must, then lest's just create a non-generic class with the same name as the generic class, make it static and have it contain those create methods. No need to pollute namespace with different confusing names. In reply to: 187230636 [](ancestors = 187230636,187226013,187223445,187223070,187222607,187222326)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Honestly I'd rather just not expose the implementations at all, have the static create methods return an object of the appropriate interface. I don't think anything is gained by exposing them. In reply to: 187240158 [](ancestors = 187240158,187230636,187226013,187223445,187223070,187222607,187222326)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In reply to: 187352180 [](ancestors = 187352180,187240158,187230636,187226013,187223445,187223070,187222607,187222326) |
||
| { | ||
| /// <summary> | ||
| /// Creates memory collection loader. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
loader from IList #Closed
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that not obvious from the parameter type? Perhaps adding a "from ." or somesuch would be fine (added to both), but even that I'd say would be a bit too needlessly verbose. In reply to: 187221474 [](ancestors = 187221474)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's confusing that it says "Collection Loader" while it takes a List . perhaps we should rename it to a ListLoader. and split away from the StreamingEnumerableLoader In reply to: 187230849 [](ancestors = 187230849,187221474)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. User no longer see class, it just get interface, is it better? In reply to: 187238467 [](ancestors = 187238467,187230849,187221474) |
||
| /// </summary> | ||
| public static ILearningPipelineLoader Create<T>(IList<T> data) where T : class | ||
| { | ||
| return new MemoryCollectionLoader<T>(data); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Creates memory collection loader. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
loader from IEnumerable. #Closed |
||
| /// </summary> | ||
| public static ILearningPipelineLoader Create<T>(IEnumerable<T> data) where T : class | ||
| { | ||
| return new MemoryCollectionLoader<T>(data); | ||
| } | ||
|
|
||
|
|
||
| /// <summary> | ||
| /// Allows you to convert your memory collection into IDataview. | ||
| /// </summary> | ||
| /// <typeparam name="TInput"></typeparam> | ||
| private class MemoryCollectionLoader<TInput> : ILearningPipelineLoader | ||
| where TInput : class | ||
| { | ||
| public ILearningPipelineStep ApplyStep(ILearningPipelineStep previousStep, Experiment experiment) | ||
| { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| 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; | ||
|
|
||
| /// <summary> | ||
| /// Creates IDataview on top of collection | ||
| /// </summary> | ||
| public MemoryCollectionLoader(IList<TInput> collection) | ||
| { | ||
| Contracts.CheckParamValue(Utils.Size(collection) > 0, collection, nameof(collection), "Must be non-empty"); | ||
| _listCollection = collection; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Creates IDataview on top of collection | ||
| /// </summary> | ||
| public MemoryCollectionLoader(IEnumerable<TInput> collection) | ||
| { | ||
| Contracts.CheckValue(collection, nameof(collection)); | ||
| _enumerableCollection = collection; | ||
|
|
||
| } | ||
|
|
||
| public void SetInput(IHostEnvironment env, Experiment experiment) | ||
| { | ||
| if (_listCollection != null) | ||
| _dataView = ComponentCreation.CreateDataView(env, _listCollection); | ||
| if (_enumerableCollection != null) | ||
| _dataView = ComponentCreation.CreateStreamingDataView(env, _enumerableCollection); | ||
| env.CheckValue(_dataView, nameof(_dataView)); | ||
| experiment.SetInput(_dataViewEntryPoint.Data, _dataView); | ||
| } | ||
|
|
||
| private class MemoryCollectionPipelineStep : ILearningPipelineDataStep | ||
| { | ||
| public MemoryCollectionPipelineStep(Var<IDataView> data) | ||
| { | ||
| Data = data; | ||
| } | ||
|
|
||
| public Var<IDataView> Data { get; } | ||
| public Var<ITransformModel> Model => null; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| // 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.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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Where is this being used? I don't see any references to this class in code or tests. #Closed
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a entrypoint :) Data.DataViewReference it get used in MemoryCollection.cs (At least it entry point wrapper) In reply to: 187222065 [](ancestors = 187222065)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, silly me as looking for "InMemoryDataView" not "Data.DataViewReference" In reply to: 187222365 [](ancestors = 187222365,187222065)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So to clarify, all this entrypoint does it turns input to output? Should we call it as such, something like a data passthrough or something?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No it doesn't. but I wouldn't call it DataPass entrypoint either, since it allow you pass only dataview from you code to experiment, and DataViewReference is already taken by entrypoint class. In reply to: 187240561 [](ancestors = 187240561)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not DataViewReference or DataViewReferenceEp? Seems like very unrelated class name to the entrypoint name. In reply to: 187395356 [](ancestors = 187395356,187240561) |
||
| { | ||
| public sealed class Input | ||
| { | ||
| [Argument(ArgumentType.Required, ShortName = "data", HelpText = "Pointer to IDataView in memory", SortOrder = 1)] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Since shortname is same as longname, you can safely omit. #Resolved |
||
| 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 }; | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ML.Data #Closed
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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