Skip to content

Change IModelCombiner to not be generic, and add unit tests #1305

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 4 commits into from
Oct 23, 2018

Conversation

yaeldekel
Copy link

Fixes #1304.

Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka left a comment

Choose a reason for hiding this comment

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

:shipit:

CombineAndTestEnsembles(dataView, "pe", "oc=average", PredictionKind.BinaryClassification, predictors);
}

[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // x86 fails with "Unknown command: 'train'; Format error at (83,3)-(83,4011): Illegal quoting"
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Oct 22, 2018

Choose a reason for hiding this comment

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

// x86 fails with "Unknown command: 'train'; Format error at (83,3)-(83,4011): Illegal quoting" [](start = 83, length = 95)

Can we have issue about this? it's weird exception, I don't get how change of platform can lead to change of reading data. #Resolved

Copy link
Author

@yaeldekel yaeldekel Oct 23, 2018

Choose a reason for hiding this comment

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

I copied this comment from the other tests in this file, I'm not sure it actually fails on x86.
@tannergooding , is there an issue for this? #Resolved

Copy link
Member

@tannergooding tannergooding Oct 23, 2018

Choose a reason for hiding this comment

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

#1216 #Resolved

var inputFile = new SimpleFileHandle(Env, dataPath, false, false);
#pragma warning disable 0618
var dataView = ImportTextData.ImportText(Env, new ImportTextData.Input { InputFile = inputFile }).Data;
#pragma warning restore 0618
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Oct 22, 2018

Choose a reason for hiding this comment

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

Can we not add obsolete code?

            var reader = TextLoader.CreateReader(Env, ctx => (
                Features: ctx.LoadFloat(1, 8),
                Label: ctx.LoadFloat(0)
            ));
            var data = reader.Read(dataPath).AsDynamic;

``` #Resolved

@@ -70,6 +73,12 @@ public EnsembleTrainer(IHostEnvironment env, Arguments args)
Combiner = args.OutputCombiner.CreateComponent(Host);
}

public EnsembleTrainer(IHostEnvironment env, Arguments args, PredictionKind predictionKind)
Copy link
Member

@sfilipi sfilipi Oct 22, 2018

Choose a reason for hiding this comment

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

public EnsembleTrainer(IHostEnvironment env, Arguments args, PredictionKind predictionKind) [](start = 6, length = 93)

in the spirit of reducing public ctor, would it make sense to add the PredictionKind as an optional param, or that would be confusing? #Resolved

Copy link
Author

Choose a reason for hiding this comment

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

I made it private :-).


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

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:

@yaeldekel yaeldekel merged commit 1dff4c2 into dotnet:master Oct 23, 2018
@yaeldekel yaeldekel deleted the modelcombiner branch October 23, 2018 18:36
@ghost ghost locked as resolved and limited conversation to collaborators Mar 27, 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.

5 participants