Towards #2326 - removing some namespaces #2442
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yaeldekel
reviewed
Feb 7, 2019
| @@ -2,7 +2,7 @@ | |||
| // The .NET Foundation licenses this file to you under the MIT license. | |||
| // See the LICENSE file in the project root for more information. | |||
|
|
|||
| namespace Microsoft.ML.Trainers.FastTree.Internal | |||
| namespace Microsoft.ML.Trainers.FastTree | |||
| { | |||
| #if OLD_DATALOAD | |||
There was a problem hiding this comment.
OLD_DATALOAD [](start = 4, length = 12)
Is this whole file inside the #if? #Resolved
There was a problem hiding this comment.
I think we could in principle safely remove this, but I'd make that a separate PR. #Resolved
There was a problem hiding this comment.
There was a problem hiding this comment.
yaeldekel
reviewed
Feb 7, 2019
| @@ -2,7 +2,7 @@ | |||
| // The .NET Foundation licenses this file to you under the MIT license. | |||
| // See the LICENSE file in the project root for more information. | |||
|
|
|||
| namespace Microsoft.ML.Trainers.FastTree.Internal | |||
| namespace Microsoft.ML.Trainers.FastTree | |||
| { | |||
| #if OLD_DATALOAD | |||
There was a problem hiding this comment.
OLD_DATALOAD [](start = 4, length = 12)
Seems like all the files in this folder contain only grayed out code. #Resolved
|
@yaeldekel @TomFinley when you have a chance, lmk if this looks good or anything else needs to be changed. |
rogancarr
reviewed
Feb 9, 2019
| using Microsoft.ML.Ensemble.Selector; | ||
| using Microsoft.ML.Ensemble.Selector.FeatureSelector; | ||
| using Microsoft.ML.EntryPoints; | ||
|
|
||
| [assembly: EntryPointModule(typeof(AllFeatureSelectorFactory))] | ||
| [assembly: EntryPointModule(typeof(RandomFeatureSelector))] | ||
|
|
||
| namespace Microsoft.ML.Ensemble.EntryPoints | ||
| namespace Microsoft.ML.Ensemble | ||
| { | ||
| [TlcModule.Component(Name = AllFeatureSelector.LoadName, FriendlyName = AllFeatureSelector.UserName)] | ||
| public sealed class AllFeatureSelectorFactory : ISupportFeatureSelectorFactory |
There was a problem hiding this comment.
Does this belong in Ensemble?
rogancarr
reviewed
Feb 9, 2019
| @@ -94,7 +94,7 @@ public void FastTreeClassificationIntrospectiveTraining() | |||
| public void FastForestRegressionIntrospectiveTraining() | |||
| { | |||
| var ml = new MLContext(seed: 1, conc: 1); | |||
| var data = DatasetUtils.GenerateFloatLabelFloatFeatureVectorSamples(1000); | |||
| var data = SamplesUtils.DatasetUtils.GenerateFloatLabelFloatFeatureVectorSamples(1000); | |||
There was a problem hiding this comment.
You're alreading using SamplesUtils. Maybe drop the namespace ref?
rogancarr
reviewed
Feb 9, 2019
| @@ -18,7 +17,7 @@ public partial class TrainerEstimators | |||
| public void TreeEnsembleFeaturizerOutputSchemaTest() | |||
| { | |||
| // Create data set | |||
| var data = DatasetUtils.GenerateBinaryLabelFloatFeatureVectorSamples(1000).ToList(); | |||
| var data = SamplesUtils.DatasetUtils.GenerateBinaryLabelFloatFeatureVectorSamples(1000).ToList(); | |||
rogancarr
reviewed
Feb 9, 2019
| @@ -331,7 +331,7 @@ private void LightGbmHelper(bool useSoftmax, out string modelString, out List<Gb | |||
| floatLabels[i] = labels[i]; | |||
|
|
|||
| // Allocate LightGBM data container (called Dataset in LightGBM world). | |||
| var gbmDataSet = new Dataset(sampleValueGroupedByColumn, sampleIndicesGroupedByColumn, _columnNumber, sampleNonZeroCntPerColumn, _rowNumber, _rowNumber, "", floatLabels); | |||
| var gbmDataSet = new LightGBM.Dataset(sampleValueGroupedByColumn, sampleIndicesGroupedByColumn, _columnNumber, sampleNonZeroCntPerColumn, _rowNumber, _rowNumber, "", floatLabels); | |||
There was a problem hiding this comment.
Do you need the explicit namespace ref?
rogancarr
approved these changes
Feb 9, 2019
…the Microsoft.ML.Trainers.FastTree.Internal namespaces.
sfilipi
force-pushed
the
namespaceNitting
branch
from
7fff634 to
2b60c6d
Compare
Codecov Report
@@ Coverage Diff @@
## master #2442 +/- ##
==========================================
- Coverage 71.24% 71.23% -0.01%
==========================================
Files 788 788
Lines 141131 141131
Branches 16115 16115
==========================================
- Hits 100547 100541 -6
- Misses 36119 36124 +5
- Partials 4465 4466 +1
|
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replacing the following namespaces as described in #2326:
Microsoft.ML.Internal.Internallearn.ResultProcessor
Microsoft.ML.Trainers.FastTree.Internal,
Microsoft.ML.Learners,
Microsoft.ML.Trainers.SymSGD.