Skip to content

Creation of components through MLContext and cleanup (GcnNorm, LpNorm, RandomFourier, CustomStopWords, VectorWhiten, PCA) #2366

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
Feb 6, 2019

Conversation

artidoro
Copy link
Contributor

@artidoro artidoro commented Feb 1, 2019

This PR is part of the work outlined in #1798, and focuses on the GcnNorm, LpNorm, RandomFourier, CustomStopWords, VectorWhiten, PCA transformers/estimators:

  1. Internalize constructors of estimators and transformers
  2. Keep ColumnInfo and move it to to the estimators The ColumnInfo structure should live in the estimators, rather than transformers #1760
  3. Rename Arguments -> Options
  4. Internalize Options only when they are not used by public constructor. For all other cases, retain Options as public Arguments class should be made internal when possible #1758

fixes #2275

If I wrote (not a ITransformer) on the #1798 issue, and this PR number appears on the same line, I renamed the Arguments to Options, and tried to make it internal.

@codecov
Copy link

codecov bot commented Feb 1, 2019

Codecov Report

Merging #2366 into master will decrease coverage by 0.08%.
The diff coverage is 78.07%.

@@            Coverage Diff             @@
##           master    #2366      +/-   ##
==========================================
- Coverage   71.26%   71.17%   -0.09%     
==========================================
  Files         785      780       -5     
  Lines      140946   140404     -542     
  Branches    16108    16053      -55     
==========================================
- Hits       100440    99936     -504     
+ Misses      36039    36017      -22     
+ Partials     4467     4451      -16
Flag Coverage Δ
#Debug 71.17% <78.07%> (-0.09%) ⬇️
#production 67.56% <75.94%> (-0.05%) ⬇️
#test 85.18% <100%> (-0.14%) ⬇️

@Ivanidzo4ka
Copy link
Contributor

Ivanidzo4ka commented Feb 1, 2019

    public VectorWhiteningTransformer Fit(IDataView input)
/// <summary>
    /// Train and return a transformer.
    /// </summary> #Closed

Refers to: src/Microsoft.ML.HalLearners/VectorWhitening.cs:806 in 78a287e. [](commit_id = 78a287e, deletion_comment = False)

Copy link
Contributor

@rogancarr rogancarr left a comment

Choose a reason for hiding this comment

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

LGTM.

Just a few comments about adding more information in the docs. If this isn't the appropriate PR for adding detailed docs info, can you make sure they are listed in #1209 as a to-do?

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:

Copy link
Contributor

@zeahmed zeahmed left a comment

Choose a reason for hiding this comment

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

:shipit:

@artidoro artidoro merged commit 6ec3280 into dotnet:master Feb 6, 2019
@artidoro artidoro deleted the round4 branch March 13, 2019 17:55
@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API Issues pertaining the friendly API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lockdown Microsoft.ML.PCA public surface
4 participants