Fix CodeGen command and add a unit test #1654
Conversation
| new Dictionary<string, string> | ||
| { | ||
| { "EXAMPLE_CLASS_DECL", loaderSb.ToString() }, | ||
| { "SCORED_EXAMPLE_CLASS_DECL", nonScoreSb.ToString() }, |
There was a problem hiding this comment.
SCORED_EXAMPLE_CLASS_DECL [](start = 27, length = 25)
In our internal repo we actually have this thing
// These are all remaining available columns, either supplied as the input, or intermediate
// columns generated by the transforms. Materializing these columns has a performance cost,
// so they are commented out. Feel free to uncomment any column that is useful for your scenario.
#if false
/#SCORED_EXAMPLE_CLASS_DECL#/
/#/SCORED_EXAMPLE_CLASS_DECL#/
#endif
Are you sure you want to delete it?
#Resolved
There was a problem hiding this comment.
Yep, that's strange. #Resolved
| if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
| return; | ||
|
|
||
| // REVIEW petelu: this tests that the generated output matches the baseline. This does NOT baseline |
There was a problem hiding this comment.
who is petelu?.. #Resolved
| new Dictionary<string, string> | ||
| { | ||
| { "EXAMPLE_CLASS_DECL", loaderSb.ToString() }, | ||
| { "SCORED_EXAMPLE_CLASS_DECL", nonScoreSb.ToString() }, |
There was a problem hiding this comment.
Yep, that's strange. #Resolved
| /// | ||
| ///</summary> | ||
| public static void Predict(string modelPath) | ||
| public static void PredictAsync(string modelPath) |
There was a problem hiding this comment.
PredictAsync [](start = 19, length = 12)
Did you try running this code now? I think the return type now should be Task? #Resolved
| public static void Predict(string modelPath) | ||
| public static void PredictAsync(string modelPath) | ||
| { | ||
| var model = await PredictionModel.ReadAsync<InputData, ScoredOutput>(modelPath); |
There was a problem hiding this comment.
PredictionModel [](start = 22, length = 15)
PredictionModel is in Microsoft.ML.Legacy which we don't reference in usings.
I doubt this code would compile.
Also do we actually want it to generate Legacy things or we should switch to new api?
There was a problem hiding this comment.
I opened issue #1738 to convert the command to use the new api.
| } | ||
|
|
||
| [Fact] | ||
| [Fact(Skip = "'checker' is not a valid value for the 'parag' argument in FastTree")] |
There was a problem hiding this comment.
should we provide a valid value instead of skipping the test ?
There was a problem hiding this comment.
If we do skip the test, should there be an issue filed on how to fix?
In reply to: 237247906 [](ancestors = 237247906)
| public void CommandShowSchemaModel() | ||
| { | ||
| string trainDataPath = GetDataPath(@"..\UCI", "adult.test.tiny"); | ||
| string trainDataPath = GetDataPath("adult.tiny.with-schema.txt"); |
There was a problem hiding this comment.
Can this be TestDataset.adult.testFilename?
| // will be in order. | ||
|
|
||
| // First, train a model on breast-cancer. | ||
| var dataPath = GetDataPath("breast-cancer.txt"); |
There was a problem hiding this comment.
TestDatasets.breastCancer.testFilename?
|
posted a couple of comments, but overall looks good. |
ghost
locked as resolved and limited conversation to collaborators
6 participants
Fixes #1643 .