Lockdown Microsoft.ML.FastTree public surface #2511
Conversation
|
### NEW API SURFACE: N:Microsoft.ML #Resolved |
Codecov Report
@@ Coverage Diff @@
## master #2511 +/- ##
==========================================
+ Coverage 71.53% 71.54% +<.01%
==========================================
Files 800 800
Lines 141858 141846 -12
Branches 16121 16119 -2
==========================================
+ Hits 101476 101479 +3
+ Misses 35930 35916 -14
+ Partials 4452 4451 -1
|
codemzs
force-pushed
the
fasttreesurfacearea
branch
2 times, most recently
from
a5d31a6 to
40a8d85
Compare
codemzs
force-pushed
the
fasttreesurfacearea
branch
from
40a8d85 to
f67e38e
Compare
| @@ -900,12 +899,12 @@ private double[] GetInitScores(Dataset set) | |||
|
|
|||
| internal abstract class DataConverter | |||
| { | |||
| protected readonly int NumFeatures; | |||
| private protected readonly int NumFeatures; | |||
There was a problem hiding this comment.
private [](start = 8, length = 7)
For an internal class, is there a difference between protected and private protected? #Resolved
There was a problem hiding this comment.
probably not but I don't see this change doing any harm either :-)
In reply to: 258221426 [](ancestors = 258221426)
| private protected double[] InitTrainScores; | ||
| private protected double[] InitValidScores; | ||
| private protected double[][] InitTestScores; | ||
| //internal int Iteration; |
There was a problem hiding this comment.
//internal int Iteration; [](start = 8, length = 25)
Can be deleted. #Resolved
| @@ -223,18 +223,18 @@ private IEnumerable<bool> GetClassificationLabelsFromRatings(Dataset set) | |||
| return set.Ratings.Select(x => x >= 1); | |||
| } | |||
|
|
|||
| protected override void PrepareLabels(IChannel ch) | |||
| private protected override void PrepareLabels(IChannel ch) | |||
| { | |||
| _trainSetLabels = GetClassificationLabelsFromRatings(TrainSet).ToArray(TrainSet.NumDocs); | |||
| //Here we set regression labels to what is in bin file if the values were not overriden with floats | |||
There was a problem hiding this comment.
//Here we set regression labels to what is in bin file if the values were not overriden with floats [](start = 12, length = 99)
Not related to your change, but - should this be deleted? #Resolved
There was a problem hiding this comment.
It may be, I might prefer to wait on that for now. #Resolved
| @@ -8,7 +8,7 @@ | |||
|
|
|||
| namespace Microsoft.ML | |||
| { | |||
| public sealed class QuantileStatistics : IQuantileDistribution<float> | |||
| internal sealed class QuantileStatistics : IQuantileDistribution<float> | |||
There was a problem hiding this comment.
QuantileStatistics [](start = 26, length = 18)
The IQuantileDistribution interface seems to have just this one implementation,
which only uses the GetQuantile() method. Can this be defined instead as a private class inside FastForestRegressionModelParameters? #Resolved
There was a problem hiding this comment.
That would probably be positive. Then we can delete that internal IQuantileDistribution interface altogether. A quick and easy improvement in case we need to update this PR again.
In reply to: 258227442 [](ancestors = 258227442)
There was a problem hiding this comment.
Hi thanks for doing this @codemzs.
If you have time while waiting for a second signoff, the enhancements as suggested by @yaeldekel would be nice to improve our overall code quality, and I'd re-address the issues raised by @zeahmed since certainly having public abstract and virtual methods inside an interface class are not problematic at all. 😄 But only if you have time, code quality inside FastTree is sort of just problematic already.
codemzs
force-pushed
the
fasttreesurfacearea
branch
from
f67e38e to
a29e51d
Compare
ghost
locked as resolved and limited conversation to collaborators
fixes #2266
Reduces public API count from 1098 to
274237.