Fix ResultProcessor bug, LogisticRegression bug and missing value conversion bug #1236
Conversation
yaeldekel
commented
Oct 12, 2018
•
yaeldekel
commented
- LogisticRegression doesn't pass the training stats to the predictor: fixes Logistic Regression doesn't work with training stats #1205 .
- ResultProcessor has code that doesn't build: fixes ResultProcessor.cs: code under "#if TLCFULLBUILD" doesn't build #1186 .
- Missing value conversion: fixes Conversion: TryParseKey() throws an exception #1187 .
- Sweeper needs to load all components into ComponentCatalog: fixes Sweeper: ConfigRunner runs ResultProcessor without loading any assemblies into ComponentCatalog #1188 .
| @@ -1170,7 +1170,7 @@ private bool IsStdMissing(ref ReadOnlySpan<char> span) | |||
| public bool TryParseKey(ref TX src, U8 min, U8 max, out U8 dst) | |||
| { | |||
| var span = src.Span; | |||
| Contracts.Check(!IsStdMissing(ref span), "Missing text value cannot be converted to unsigned integer type."); | |||
| Contracts.Check(span.IsEmpty || !IsStdMissing(ref span), "Missing text value cannot be converted to unsigned integer type."); | |||
There was a problem hiding this comment.
span.IsEmpty [](start = 28, length = 12)
Heh heh. Whoops! #Resolved
test/data/Mapping.de-de.txt
Outdated
| @@ -0,0 +1,7 @@ | |||
| Wirtschaft 0 | |||
There was a problem hiding this comment.
This is associated with lm.sample.txt, I guess? So I wonder if lm.labels.txt might be a better name, just to make clear the two files are in some way associated -- Mapping.de-de.txt is not entirely clear. #Resolved
| @@ -1154,13 +1154,14 @@ public void CommandTrainMlrWithLabelNames() | |||
| Done(); | |||
| } | |||
|
|
|||
| [Fact(Skip = "Need CoreTLC specific baseline update")] | |||
| //[Fact(Skip = "Need CoreTLC specific baseline update")] | |||
| [Fact] | |||
There was a problem hiding this comment.
make your mind :) #Closed
| @@ -0,0 +1 @@ | |||
| Saving predictor summary | |||
There was a problem hiding this comment.
summary [](start = 17, length = 7)
with latest Eric changes (#1193) you can just use Common folder if files for Debug and Release are same. #Closed
| TestCore(pathData, false, | ||
| new[] { | ||
| "loader=Text{header+ sep=comma col=Label:14 col=Age:0 col=Gender:TX:9 col=Mar:TX:5 col=Race:TX:8 col=Num:2,4,10-12 col=Txt:TX:~}", | ||
| "xf=Cat{col=Race2:Key:Race data={" + pathTerms + "} termCol=Whatever}", |
There was a problem hiding this comment.
Race2:Key:Race [](start = 32, length = 14)
Is this supported? #Closed
There was a problem hiding this comment.
Are you referring to the "Race2:Key:Race" syntax? Or to the term data file argument? The first is supported, the second is supported in the latest iteration of this PR :-).
In reply to: 224882266 [](ancestors = 224882266)
| string pathData = GetDataPath(@"lm.sample.txt"); | ||
| string mappingPathData = GetDataPath(@"lm.labels.txt"); | ||
|
|
||
| // REVIEW shonk: The file doesn't really have a header row. Is it intentional to pretend it does? |
There was a problem hiding this comment.
shonk [](start = 22, length = 5)
think we don't use aliases here anymore. #Resolved
There was a problem hiding this comment.
| { | ||
| } | ||
|
|
||
| public CategoricalEstimator(IHostEnvironment env, params ColumnInfo[] columns) | ||
| public CategoricalEstimator(IHostEnvironment env, ColumnInfo[] columns, |
There was a problem hiding this comment.
public [](start = 8, length = 6)
out of curiosity can you make this constructor internal? I would prefer to not pollute our public API with these things.
Same for TermEstimator.
ghost
locked as resolved and limited conversation to collaborators
