Skip to content

Cleaned and fixed public API surface for KMeans, NaiveBayes, OLS. #2819

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 5 commits into from
Mar 5, 2019

Conversation

zeahmed
Copy link
Contributor

@zeahmed zeahmed commented Mar 1, 2019

This is the final PR related to #2620. This PR finally fixes #2620

The following learners are addressed in this PR.

the following tasks were performed in classes related to above learners.

Checking to make sure that unnecessary public methods/properties be internal.
Renaming parameters according to standard.
Creating/Refactoring samples according to standards.

@zeahmed zeahmed requested review from sfilipi and Ivanidzo4ka March 1, 2019 22:46
@zeahmed zeahmed changed the title Cleaning public API surface for KMeans, NaiveBayes, OLS. Cleaned and fixed public API surface for KMeans, NaiveBayes, OLS. Mar 1, 2019
@Ivanidzo4ka
Copy link
Contributor

Ivanidzo4ka commented Mar 1, 2019

        public float L2Weight = 1e-6f;

L2Regularization please. #Resolved


Refers to: src/Microsoft.ML.HalLearners/OlsLinearRegression.cs:50 in 7b2890b. [](commit_id = 7b2890b, deletion_comment = False)

@Ivanidzo4ka
Copy link
Contributor

Ivanidzo4ka commented Mar 1, 2019

        public bool PerParameterSignificance = true;

CalculateStatistics ? #Resolved


Refers to: src/Microsoft.ML.HalLearners/OlsLinearRegression.cs:56 in 7b2890b. [](commit_id = 7b2890b, deletion_comment = False)

@Ivanidzo4ka
Copy link
Contributor

Ivanidzo4ka commented Mar 1, 2019

        public int ClustersCount = Defaults.ClustersCount;

NumberOfClusters? #Resolved


Refers to: src/Microsoft.ML.KMeansClustering/KMeansPlusPlusTrainer.cs:61 in 7b2890b. [](commit_id = 7b2890b, deletion_comment = False)

@Ivanidzo4ka
Copy link
Contributor

Ivanidzo4ka commented Mar 1, 2019

        KMeansParallel = 2

KMeansYinyang ? #Resolved


Refers to: src/Microsoft.ML.KMeansClustering/KMeansPlusPlusTrainer.cs:43 in 7b2890b. [](commit_id = 7b2890b, deletion_comment = False)

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:

@Ivanidzo4ka
Copy link
Contributor

Ivanidzo4ka commented Mar 1, 2019

Small comments, otherwise looks super good! #Resolved

@codecov
Copy link

codecov bot commented Mar 1, 2019

Codecov Report

Merging #2819 into master will increase coverage by 0.01%.
The diff coverage is 91.11%.

@@            Coverage Diff             @@
##           master    #2819      +/-   ##
==========================================
+ Coverage   71.68%   71.69%   +0.01%     
==========================================
  Files         808      809       +1     
  Lines      142402   142494      +92     
  Branches    16113    16114       +1     
==========================================
+ Hits       102076   102162      +86     
- Misses      35893    35900       +7     
+ Partials     4433     4432       -1
Flag Coverage Δ
#Debug 71.69% <91.11%> (+0.01%) ⬆️
#production 67.92% <89.65%> (-0.01%) ⬇️
#test 85.9% <93.75%> (+0.04%) ⬆️
Impacted Files Coverage Δ
src/Microsoft.ML.StaticPipe/KMeansStatic.cs 43.58% <0%> (ø) ⬆️
src/Microsoft.ML.KMeansClustering/KMeansCatalog.cs 100% <100%> (ø) ⬆️
...osoft.ML.KMeansClustering/KMeansPlusPlusTrainer.cs 84.67% <100%> (ø) ⬆️
...Standard/MultiClass/MultiClassNaiveBayesTrainer.cs 87.12% <100%> (-0.69%) ⬇️
test/Microsoft.ML.Tests/OnnxConversionTest.cs 97.16% <100%> (ø) ⬆️
...crosoft.ML.Core.Tests/UnitTests/TestEntryPoints.cs 98.07% <100%> (ø) ⬆️
...ests/TrainerEstimators/OlsLinearRegressionTests.cs 100% <100%> (ø) ⬆️
...ft.ML.Tests/TrainerEstimators/TrainerEstimators.cs 92.5% <100%> (ø) ⬆️
test/Microsoft.ML.Functional.Tests/Evaluation.cs 100% <100%> (ø) ⬆️
test/Microsoft.ML.Functional.Tests/Validation.cs 100% <100%> (ø) ⬆️
... and 20 more

@@ -9,7 +9,7 @@ public static void Example()
{
// Create a new ML context, for ML.NET operations. It can be used for exception tracking and logging,
// as well as the source of randomness.
var ml = new MLContext();
var ml = new MLContext(seed: 1, conc: 1);
Copy link
Member

@sfilipi sfilipi Mar 4, 2019

Choose a reason for hiding this comment

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

seed: 1, conc: 1 [](start = 35, length = 16)

is this needed? #Resolved

Copy link
Contributor Author

@zeahmed zeahmed Mar 4, 2019

Choose a reason for hiding this comment

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

Yes, its needed otherwise results are different on every run.


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

@sfilipi
Copy link
Member

sfilipi commented Mar 4, 2019

    /// </summary>

Please link to the KMeansPlusPlusTrainerthrough

Shahab put a template in place for how to document extensions, so we have a pointer to the main estimator where there are more explanations.
#Resolved


Refers to: src/Microsoft.ML.KMeansClustering/KMeansCatalog.cs:19 in 2a1bb29. [](commit_id = 2a1bb29, deletion_comment = False)

@sfilipi
Copy link
Member

sfilipi commented Mar 4, 2019

    /// Train a KMeans++ clustering algorithm.

similar, link to KMeansPlusPlusTrainer #Resolved


Refers to: src/Microsoft.ML.KMeansClustering/KMeansCatalog.cs:48 in 2a1bb29. [](commit_id = 2a1bb29, deletion_comment = False)

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:

@zeahmed zeahmed merged commit d9208bf into dotnet:master Mar 5, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 23, 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.

Scrubbing rest of learners
3 participants