Skip to content
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

Get rid of public tuples #2950

Merged
merged 3 commits into from Mar 14, 2019

Conversation

Projects
None yet
4 participants
@Ivanidzo4ka
Copy link
Member

Ivanidzo4ka commented Mar 13, 2019

fixes #2881

@Ivanidzo4ka Ivanidzo4ka requested review from TomFinley , sfilipi and wschin Mar 13, 2019

/// <summary>
/// Drops missing values from columns.
/// </summary>
public sealed class MissingValueDroppingEstimator : TrivialEstimator<MissingValueDroppingTransformer>

This comment has been minimized.

@wschin

wschin Mar 13, 2019

Member

Don't we need this estimator anymore? Is there any alternative? #Closed

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Mar 13, 2019

Author Member

We don't have extensions for this estimator.
We use transformer as part of other transformers, but I don't see any work with estimator.
I'm tempted to make transformer internal as well, since I don't see how it can be useful for user...


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

This comment has been minimized.

@TomFinley

TomFinley Mar 14, 2019

Contributor

Unless we have a scenario I would certainly welcome internalizing it, since we can always de-internalize it once we do have a scenario. But if we view this as too far then I'd understand that as well.

@wschin

wschin approved these changes Mar 13, 2019

Copy link
Member

wschin left a comment

LGTM.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 13, 2019

Codecov Report

Merging #2950 into master will increase coverage by 0.06%.
The diff coverage is 54.54%.

@@            Coverage Diff             @@
##           master    #2950      +/-   ##
==========================================
+ Coverage   72.19%   72.26%   +0.06%     
==========================================
  Files         796      796              
  Lines      142038   142287     +249     
  Branches    16047    16042       -5     
==========================================
+ Hits       102548   102824     +276     
+ Misses      35110    35086      -24     
+ Partials     4380     4377       -3
Flag Coverage Δ
#Debug 72.26% <54.54%> (+0.06%) ⬆️
#production 68% <54.54%> (+0.02%) ⬆️
#test 88.4% <ø> (+0.09%) ⬆️
Impacted Files Coverage Δ
...rosoft.ML.StaticPipelineTesting/StaticPipeTests.cs 95.27% <ø> (ø) ⬆️
...t.ML.Transforms/MissingValueDroppingTransformer.cs 82.23% <0%> (+9.26%) ⬆️
...soft.ML.Transforms/Text/WordEmbeddingsExtractor.cs 87.52% <100%> (-0.03%) ⬇️
src/Microsoft.ML.StaticPipe/LdaStaticExtensions.cs 94.02% <100%> (ø) ⬆️
src/Microsoft.ML.Transforms/Text/LdaTransform.cs 89.26% <52.63%> (-0.48%) ⬇️
...c/Microsoft.ML.FastTree/Utils/ThreadTaskManager.cs 79.48% <0%> (-20.52%) ⬇️
src/Microsoft.ML.Maml/MAML.cs 24.75% <0%> (-1.46%) ⬇️
...soft.ML.TestFramework/DataPipe/TestDataPipeBase.cs 73.67% <0%> (-0.34%) ⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs 85.53% <0%> (-0.17%) ⬇️
src/Microsoft.ML.Data/Transforms/Normalizer.cs 84.75% <0%> (-0.07%) ⬇️
... and 16 more
@@ -20,8 +20,8 @@ public sealed class LatentDirichletAllocationFitResult
/// <param name="result"></param>
public delegate void OnFit(LatentDirichletAllocationFitResult result);

public LatentDirichletAllocationTransformer.LdaSummary LdaTopicSummary;
public LatentDirichletAllocationFitResult(LatentDirichletAllocationTransformer.LdaSummary ldaTopicSummary)
public LatentDirichletAllocationTransformer.LdaModelParameters LdaTopicSummary;

This comment has been minimized.

@singlis

singlis Mar 14, 2019

Member

LdaModelParameters [](start = 52, length = 18)

Since the model parameters is an internal class, why not call it ModelParameters instead of LdaModelParameters?

@singlis
Copy link
Member

singlis left a comment

:shipit:

@TomFinley
Copy link
Contributor

TomFinley left a comment

Thank you @Ivanidzo4ka !

@TomFinley TomFinley merged commit 4c29a09 into dotnet:master Mar 14, 2019

3 checks passed

MachineLearning-CI #20190314.4 succeeded
Details
MachineLearning-CodeCoverage #20190314.4 succeeded
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.