Scrub projection transforms #2865

wschin · 2019-03-06T01:35:49Z

Transforms touched:
RFF, LpNorm, GcNorm, PCA, Whiten, Normalize.

codecov · 2019-03-06T02:46:03Z

Codecov Report

Merging #2865 into master will decrease coverage by <.01%.
The diff coverage is 86.34%.

@@            Coverage Diff             @@
##           master    #2865      +/-   ##
==========================================
- Coverage   71.79%   71.79%   -0.01%     
==========================================
  Files         812      812              
  Lines      142680   142673       -7     
  Branches    16124    16124              
==========================================
- Hits       102436   102430       -6     
  Misses      35831    35831              
+ Partials     4413     4412       -1

Flag	Coverage Δ
#Debug	`71.79% <86.34%> (-0.01%)`	⬇️
#production	`67.92% <83.85%> (ø)`	⬆️
#test	`86.23% <95.45%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
.../Microsoft.ML.Data/Transforms/TransformsCatalog.cs	`100% <ø> (ø)`	⬆️
...rosoft.ML.Transforms/FourierDistributionSampler.cs	`84.16% <ø> (ø)`	⬆️
test/Microsoft.ML.Benchmarks/RffTransform.cs	`0% <0%> (ø)`	⬆️
...s/StochasticDualCoordinateAscentClassifierBench.cs	`0% <0%> (ø)`	⬆️
...icrosoft.ML.Mkl.Components/MklComponentsCatalog.cs	`94.28% <0%> (ø)`	⬆️
...icrosoft.ML.Functional.Tests/DataTransformation.cs	`100% <100%> (ø)`	⬆️
src/Microsoft.ML.Data/Transforms/Normalizer.cs	`84.81% <100%> (ø)`	⬆️
...ents.StaticPipe/VectorWhiteningStaticExtensions.cs	`100% <100%> (ø)`	⬆️
...ft.ML.Tests/TrainerEstimators/TrainerEstimators.cs	`92.5% <100%> (ø)`	⬆️
test/Microsoft.ML.Tests/Transformers/RffTests.cs	`100% <100%> (ø)`	⬆️
... and 24 more

sfilipi · 2019-03-06T04:08:05Z

src/Microsoft.ML.PCA/PcaTrainer.cs

-            string features,
-            string weights = null,
+            string featureColumnName,
+            string exampleWeightColumnName = null,


exampleWeightColumnName [](start = 19, length = 23)

does 'example' need to be in the name? just too long.. #WontFix

Yes but it's used everywhere. I will open an issue (#2873) instead.

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

sfilipi · 2019-03-06T04:27:23Z

src/Microsoft.ML.StaticPipe/TransformsStatic.cs

-        /// <param name="newDim">Expected size of new vector.</param>
-        /// <param name="useSin">Create two features for every random Fourier frequency? (one for cos and one for sin) </param>
+        /// <param name="dimension">The number of random Fourier features to create.</param>
+        /// <param name="useCosAndSinBases">If <see langword="true"/>, use both of cos and sin basis functions to create two features for every random Fourier frequency.  /// Otherwise, only cos bases would be used.</param>


/// Otherwise, only cos bases would be used. [](start = 171, length = 52)

new line #Resolved

sfilipi

Ivanidzo4ka · 2019-03-06T04:52:42Z

src/Microsoft.ML.PCA/PCACatalog.cs

@@ -18,18 +18,18 @@ public static class PcaCatalog
        /// <param name="exampleWeightColumnName">The name of the example weight column (optional).</param>
        /// <param name="rank">The number of principal components.</param>


Would be nice to have consistency among transforms which lower feature vector dimension.
Right now we have dimension and rank and pcaNum in whitening.
#Resolved

Done.

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

artidoro · 2019-03-07T00:44:39Z

I know @TomFinley has some opinions about how MLContext should be organized.
In your PR you remove the Transforms.Projection catalog section and instead create three others (I also added the transforms that go in each section so it's easier to review):

Transforms.DimensionReduction
- VectorWhitening
- PCA
Transforms.KernelExpansion
- CreateRandomFourierFeatures
Transforms.Normalization
- Normalize
- LpNormalize
- GlobalContrastNormalize

Since this introduces quite a few sections I think @TomFinley should take a look before we can check it in. #Resolved

artidoro · 2019-03-07T00:48:30Z

src/Microsoft.ML.Transforms/KernelCatalog.cs

+        /// <param name="outputColumnName">Name of the column resulting from the transformation of <paramref name="inputColumnName"/>.</param>
+        /// <param name="inputColumnName">Name of column to transform. If set to <see langword="null"/>, the value of the <paramref name="outputColumnName"/> will be used as source.</param>
+        /// <param name="dimension">The number of random Fourier features to create.</param>
+        /// <param name="useCosAndSinBases">If <see langword="true"/>, use both of cos and sin basis functions to create two features for every random Fourier frequency.  /// Otherwise, only cos bases would be used.</param>


/// [](start = 171, length = 3)

new line #Resolved

Nice catch.

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

artidoro · 2019-03-07T00:52:27Z

src/Microsoft.ML.Transforms/KernelCatalog.cs

+        /// ]]>
+        /// </format>
+        /// </example>
+        public static RandomFourierExpansionEstimator RandomFourierExpand(this TransformsCatalog.KernelExpansionTransforms catalog,


RandomFourierExpansionEstimator [](start = 22, length = 31)

Why Random? I like the Expansion part, but why Random? I don't think the process it that random right? #Resolved

Formal name: randomized Fourier kernel approximation.

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

TomFinley · 2019-03-07T00:58:20Z

Since this introduces quite a few sections I think @TomFinley should take a look before we can check it in.

Well, I feel like this is introducing too much categorization @wschin and @artidoro. So if the categories start to have one or two items, then I wonder what the point of categorization is. So why not just not categorize if we get to that point? Does anything really bad happen if transforms isn't endlessly categorized? This is starting to resemble a Linnaeusian taxonomy.

I see a few end states with categorization. We have in this PR at the time I am writing descriptive categories, but they are too fine grained. We had previously categories that were, while accurate descriptions, utterly useless in terms of communicating anything to users (projection). The alternative is that we don't categorize these at all, and so I wonder if that's the most desirable state if we can't come up with good descriptive categories that don't suffer from having only one or two items in them.

So how bad is it if we just put these things in transforms? #Resolved

wschin · 2019-03-07T01:19:30Z

Not bad! Let's do this.

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

artidoro

artidoro · 2019-03-07T04:28:34Z

src/Microsoft.ML.Transforms/NormalizerCatalog.cs

+        /// </example>
+        public static NormalizingEstimator Normalize(this TransformsCatalog catalog,
+           string outputColumnName, string inputColumnName = null,
+            NormalizingEstimator.NormalizerMode mode = NormalizingEstimator.NormalizerMode.MinMax)


NormalizerMode [](start = 33, length = 14)

How about NormalizationMode? #Resolved

Ok.

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

artidoro · 2019-03-07T04:28:50Z

src/Microsoft.ML.Transforms/NormalizerCatalog.cs

+        /// </format>
+        /// </example>
+        public static NormalizingEstimator Normalize(this TransformsCatalog catalog,
+            NormalizingEstimator.NormalizerMode mode,


NormalizerMode [](start = 33, length = 14)

NormalizationMode? #Resolved

Ok.

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

Ivanidzo4ka · 2019-03-07T20:27:51Z

        public string WeightColumn = PrincipalComponentAnalysisEstimator.Defaults.WeightColumn;

Can we update it to our default exampleWeightsColumnName i think. #Resolved

Refers to: src/Microsoft.ML.PCA/PcaTransformer.cs:44 in d4edc48. [](commit_id = d4edc48, deletion_comment = False)

Ivanidzo4ka · 2019-03-07T20:31:04Z

Need better names.
NormKind is not great. StdDev is not great, Linf is not great. #Resolved

Refers to: src/Microsoft.ML.Transforms/GcnTransform.cs:657 in d4edc48. [](commit_id = d4edc48, deletion_comment = False)

docs/samples/Microsoft.ML.Samples/Dynamic/ProjectionTransforms.cs

Ivanidzo4ka · 2019-03-07T20:47:11Z

    Pca,

PrincipalComponentAnalysis
and
ZeroPhaseComponentAnalysis
#Resolved

Refers to: src/Microsoft.ML.Mkl.Components/VectorWhitening.cs:42 in d4edc48. [](commit_id = d4edc48, deletion_comment = False)

Ivanidzo4ka · 2019-03-07T20:49:29Z

    public static RandomizedPcaTrainer RandomizedPca(this AnomalyDetectionCatalog.AnomalyDetectionTrainers catalog,

ApproximatePrincipalComponentsAnalysis? #Resolved

Refers to: src/Microsoft.ML.PCA/PCACatalog.cs:56 in d4edc48. [](commit_id = d4edc48, deletion_comment = False)

Ivanidzo4ka · 2019-03-07T20:55:08Z

public sealed class RandomizedPcaTrainer : TrainerEstimatorBase<AnomalyPredictionTransformer<PcaModelParameters>, PcaModelParameters>

can we move away from PCA to PrincipalComponentAnalysis ? #Resolved

Refers to: src/Microsoft.ML.PCA/PcaTrainer.cs:42 in d4edc48. [](commit_id = d4edc48, deletion_comment = False)

wschin · 2019-03-07T22:38:08Z

    public static RandomizedPcaTrainer RandomizedPca(this AnomalyDetectionCatalog.AnomalyDetectionTrainers catalog,

Will do AnalyzeRandomizedPrincipleComponents

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

Refers to: src/Microsoft.ML.PCA/PCACatalog.cs:56 in d4edc48. [](commit_id = d4edc48, deletion_comment = False)

wschin · 2019-03-07T22:44:42Z

public sealed class RandomizedPcaTrainer : TrainerEstimatorBase<AnomalyPredictionTransformer<PcaModelParameters>, PcaModelParameters>

Sure. I will replace RandomizedPcaTrainer with RandomizedPrincipalComponentAnalyzer.

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

Refers to: src/Microsoft.ML.PCA/PcaTrainer.cs:42 in d4edc48. [](commit_id = d4edc48, deletion_comment = False)

wschin · 2019-03-07T22:48:14Z

    Pca,

Sure.

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

Refers to: src/Microsoft.ML.Mkl.Components/VectorWhitening.cs:42 in d4edc48. [](commit_id = d4edc48, deletion_comment = False)

wschin · 2019-03-07T22:51:51Z

How about

        public enum NormFunction : byte
        {
            L2Norm = 0,
            StandardDeviation = 1,
            L1Norm = 2,
            InfinityNorm = 3

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

Refers to: src/Microsoft.ML.Transforms/GcnTransform.cs:657 in d4edc48. [](commit_id = d4edc48, deletion_comment = False)

wschin · 2019-03-07T22:55:07Z

        public string WeightColumn = PrincipalComponentAnalysisEstimator.Defaults.WeightColumn;

Sure. Btw, it should be "E" and without "s".

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

Refers to: src/Microsoft.ML.PCA/PcaTransformer.cs:44 in d4edc48. [](commit_id = d4edc48, deletion_comment = False)

wschin added 3 commits March 5, 2019 16:14

Clean RFT

f3f9c76

Polish LpNorm normalizer

86e5c19

Polish GCN

22e4b5e

wschin changed the title ~~Scrub norm~~ Scrub projection transforms Mar 6, 2019

wschin requested review from Ivanidzo4ka, artidoro and sfilipi and removed request for artidoro March 6, 2019 01:36

wschin changed the title ~~Scrub projection transforms~~ [WIP] Scrub projection transforms Mar 6, 2019

Work on PCA and static transforms

f95686b

sfilipi reviewed Mar 6, 2019

View reviewed changes

sfilipi approved these changes Mar 6, 2019

View reviewed changes

Ivanidzo4ka reviewed Mar 6, 2019

View reviewed changes

wschin added 2 commits March 6, 2019 14:10

Create new catalogs for normalization and kernel approximation methods

70ed227

Address comments

8b50306

wschin changed the title ~~[WIP] Scrub projection transforms~~ Scrub projection transforms Mar 6, 2019

wschin requested review from artidoro and Ivanidzo4ka March 6, 2019 23:15

A even better catalog name for RFF

1f9ae32

artidoro reviewed Mar 7, 2019

View reviewed changes

Rename Rff entirely

df08e28

artidoro reviewed Mar 7, 2019

View reviewed changes

Address comments. Everything touched is under TransformsCatalog now.

d4edc48

artidoro approved these changes Mar 7, 2019

View reviewed changes

artidoro reviewed Mar 7, 2019

View reviewed changes

Ivanidzo4ka reviewed Mar 7, 2019

View reviewed changes

docs/samples/Microsoft.ML.Samples/Dynamic/ProjectionTransforms.cs Outdated Show resolved Hide resolved

Ivanidzo4ka reviewed Mar 7, 2019

View reviewed changes

docs/samples/Microsoft.ML.Samples/Dynamic/ProjectionTransforms.cs Outdated Show resolved Hide resolved

wschin and others added 5 commits March 7, 2019 15:02

Address comments

a2e9ea5

Address comments

cebc3c6

Merge branch 'master' into scrub-norm

df71060

Fix build

22e14e7

UPdate EP

de3622a

wschin merged commit f09b25f into dotnet:master Mar 8, 2019

wschin deleted the scrub-norm branch March 8, 2019 03:47

wschin mentioned this pull request Mar 8, 2019

More Normalizer Scrubbing #2888

Merged

ghost locked as resolved and limited conversation to collaborators Mar 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scrub projection transforms #2865

Scrub projection transforms #2865

wschin commented Mar 6, 2019 •

edited

Loading

codecov bot commented Mar 6, 2019 •

edited

Loading

sfilipi Mar 6, 2019 •

edited by wschin

Loading

wschin Mar 6, 2019 •

edited

Loading

sfilipi Mar 6, 2019 •

edited by wschin

Loading

sfilipi left a comment

Ivanidzo4ka Mar 6, 2019 •

edited by wschin

Loading

wschin Mar 6, 2019

artidoro commented Mar 7, 2019 •

edited by wschin

Loading

artidoro Mar 7, 2019 •

edited by wschin

Loading

wschin Mar 7, 2019

artidoro Mar 7, 2019 •

edited by wschin

Loading

wschin Mar 7, 2019

TomFinley commented Mar 7, 2019 •

edited by wschin

Loading

wschin commented Mar 7, 2019

artidoro left a comment

artidoro Mar 7, 2019 •

edited by wschin

Loading

wschin Mar 7, 2019

artidoro Mar 7, 2019 •

edited by wschin

Loading

wschin Mar 7, 2019

Ivanidzo4ka commented Mar 7, 2019 •

edited by wschin

Loading

Ivanidzo4ka commented Mar 7, 2019 •

edited

Loading

Ivanidzo4ka commented Mar 7, 2019 •

edited by wschin

Loading

Ivanidzo4ka commented Mar 7, 2019 •

edited by wschin

Loading

Ivanidzo4ka commented Mar 7, 2019 •

edited by wschin

Loading

wschin commented Mar 7, 2019

wschin commented Mar 7, 2019

wschin commented Mar 7, 2019

wschin commented Mar 7, 2019

wschin commented Mar 7, 2019

		@@ -18,18 +18,18 @@ public static class PcaCatalog
		/// <param name="exampleWeightColumnName">The name of the example weight column (optional).</param>
		/// <param name="rank">The number of principal components.</param>

Scrub projection transforms #2865

Scrub projection transforms #2865

Conversation

wschin commented Mar 6, 2019 • edited Loading

codecov bot commented Mar 6, 2019 • edited Loading

Codecov Report

sfilipi Mar 6, 2019 • edited by wschin Loading

Choose a reason for hiding this comment

wschin Mar 6, 2019 • edited Loading

Choose a reason for hiding this comment

sfilipi Mar 6, 2019 • edited by wschin Loading

Choose a reason for hiding this comment

sfilipi left a comment

Choose a reason for hiding this comment

Ivanidzo4ka Mar 6, 2019 • edited by wschin Loading

Choose a reason for hiding this comment

wschin Mar 6, 2019

Choose a reason for hiding this comment

artidoro commented Mar 7, 2019 • edited by wschin Loading

artidoro Mar 7, 2019 • edited by wschin Loading

Choose a reason for hiding this comment

wschin Mar 7, 2019

Choose a reason for hiding this comment

artidoro Mar 7, 2019 • edited by wschin Loading

Choose a reason for hiding this comment

wschin Mar 7, 2019

Choose a reason for hiding this comment

TomFinley commented Mar 7, 2019 • edited by wschin Loading

wschin commented Mar 7, 2019

artidoro left a comment

Choose a reason for hiding this comment

artidoro Mar 7, 2019 • edited by wschin Loading

Choose a reason for hiding this comment

wschin Mar 7, 2019

Choose a reason for hiding this comment

artidoro Mar 7, 2019 • edited by wschin Loading

Choose a reason for hiding this comment

wschin Mar 7, 2019

Choose a reason for hiding this comment

Ivanidzo4ka commented Mar 7, 2019 • edited by wschin Loading

Ivanidzo4ka commented Mar 7, 2019 • edited Loading

Ivanidzo4ka commented Mar 7, 2019 • edited by wschin Loading

Ivanidzo4ka commented Mar 7, 2019 • edited by wschin Loading

Ivanidzo4ka commented Mar 7, 2019 • edited by wschin Loading

wschin commented Mar 7, 2019

wschin commented Mar 7, 2019

wschin commented Mar 7, 2019

wschin commented Mar 7, 2019

wschin commented Mar 7, 2019

wschin commented Mar 6, 2019 •

edited

Loading

codecov bot commented Mar 6, 2019 •

edited

Loading

sfilipi Mar 6, 2019 •

edited by wschin

Loading

wschin Mar 6, 2019 •

edited

Loading

sfilipi Mar 6, 2019 •

edited by wschin

Loading

Ivanidzo4ka Mar 6, 2019 •

edited by wschin

Loading

artidoro commented Mar 7, 2019 •

edited by wschin

Loading

artidoro Mar 7, 2019 •

edited by wschin

Loading

artidoro Mar 7, 2019 •

edited by wschin

Loading

TomFinley commented Mar 7, 2019 •

edited by wschin

Loading

artidoro Mar 7, 2019 •

edited by wschin

Loading

artidoro Mar 7, 2019 •

edited by wschin

Loading

Ivanidzo4ka commented Mar 7, 2019 •

edited by wschin

Loading

Ivanidzo4ka commented Mar 7, 2019 •

edited

Loading

Ivanidzo4ka commented Mar 7, 2019 •

edited by wschin

Loading

Ivanidzo4ka commented Mar 7, 2019 •

edited by wschin

Loading

Ivanidzo4ka commented Mar 7, 2019 •

edited by wschin

Loading