More Normalizer Scrubbing #2888
Conversation
wschin
changed the title
wschin
force-pushed
the
more-norm-scrub
branch
2 times, most recently
from
5709476 to
d22809e
Compare
Scrubbing normalization again
Codecov Report
@@ Coverage Diff @@
## master #2888 +/- ##
==========================================
+ Coverage 72.18% 72.19% +<.01%
==========================================
Files 796 796
Lines 142020 142032 +12
Branches 16046 16046
==========================================
+ Hits 102523 102533 +10
- Misses 35116 35117 +1
- Partials 4381 4382 +1
|
| @@ -102,35 +102,35 @@ internal static ColumnOptionsBase Create(string outputColumnName, string inputCo | |||
| } | |||
| } | |||
|
|
|||
| public abstract class FixZeroColumnOptionsBase : ColumnOptionsBase | |||
| public abstract class ControlZeroColumnOptionsBase : ColumnOptionsBase | |||
There was a problem hiding this comment.
ControlZeroColumnOptionsBase [](start = 30, length = 28)
why change this? #Pending
There was a problem hiding this comment.
FixZero is a bool option applied to zeros. Those zeros may not be fixed if FixZero=false. Thus, I feel ControlZero sounds more appropriate.
In reply to: 264105076 [](ancestors = 264105076)
There was a problem hiding this comment.
I actually think that FixZero makes more sense because it's "fixing" something in place (and not "fixing" a problem). It is a bit more precise than "control".
In reply to: 264343033 [](ancestors = 264343033,264105076)
There was a problem hiding this comment.
FixZero means zeros will not be changed right? However, if FixZero=false, zeros will be mapped to a data-dependent value (e.g., mean). It looks like we are able to change zeros to infinite amount of possibilities.
In reply to: 264825031 [](ancestors = 264825031,264343033,264105076)
There was a problem hiding this comment.
FixZero=true means that I want to fix the value of zero in place. FixZero=false means don't fix zero in place. That seems consistent, doesn't it? ControlZero doesn't convey much to me.
In reply to: 264868717 [](ancestors = 264868717,264825031,264343033,264105076)
There was a problem hiding this comment.
The problem is not only for that flag's name. When appearing in the name of FixZeroColumnOptionsBase, no one knows FixZero is a Boolean variable. Your judgement is valid only when FixZero is a field.
In reply to: 265342165 [](ancestors = 265342165,264868717,264825031,264343033,264105076)
| @@ -36,7 +36,7 @@ namespace Microsoft.ML.Transforms | |||
| /// This transformation is based on this paper by | |||
| /// <a href="http://pages.cs.wisc.edu/~brecht/papers/07.rah.rec.nips.pdf">Rahimi and Recht</a>. | |||
| /// </summary> | |||
| public sealed class RandomFourierExpansionTransformer : OneToOneTransformerBase | |||
| public sealed class RandomFourierKernelTransformer : OneToOneTransformerBase | |||
There was a problem hiding this comment.
RandomFourierKernelTransformer [](start = 24, length = 30)
I am not convinced about this change. I know what Fourier expansion is, but I have tried looking up Fourier Kernel and I have not found anything.
Also it does not seem to fit with the summary above. #Resolved
There was a problem hiding this comment.
You need to search for Random Fourier Kernel Approximation. Here is a scikit-learn example: https://scikit-learn.org/stable/modules/kernel_approximation.html
In reply to: 264411188 [](ancestors = 264411188)
There was a problem hiding this comment.
We will rename RandomFourierKernel to ApproximatedKernel everywhere (in iteration 5).
In reply to: 264443488 [](ancestors = 264443488,264411188)
| @@ -71,15 +71,15 @@ public static class NormalizationCatalog | |||
| /// </format> | |||
| /// </example> | |||
| public static LpNormalizingEstimator LpNormalize(this TransformsCatalog catalog, string outputColumnName, string inputColumnName = null, | |||
| LpNormalizingEstimatorBase.NormFunction normKind = LpNormalizingEstimatorBase.Defaults.Norm, bool ensureZeroMean = LpNormalizingEstimatorBase.Defaults.LpEnsureZeroMean) | |||
| => new LpNormalizingEstimator(CatalogUtils.GetEnvironment(catalog), outputColumnName, inputColumnName, normKind, ensureZeroMean); | |||
| LpNormalizingEstimatorBase.NormFunction norm = LpNormalizingEstimatorBase.Defaults.Norm, bool ensureZeroMean = LpNormalizingEstimatorBase.Defaults.LpEnsureZeroMean) | |||
There was a problem hiding this comment.
norm [](start = 52, length = 4)
Do you think it make sense to unify names in this one, and one in TextFeaturizingEstimator (it's VectorNormalizer) #Closed
There was a problem hiding this comment.
Their ranges are different. TextFeaturizingEstimator contains None but LpNormalizer doesn't, but it doesn't make sense to add None to the enum of LpNormalizer.
In reply to: 264822764 [](ancestors = 264822764)
There was a problem hiding this comment.
just in case, I'm not taking about type of this parameter, I'm talking about it's name.
In reply to: 264872101 [](ancestors = 264872101,264822764)
There was a problem hiding this comment.
I will use norm in TextFeaturizingEstimator.
In reply to: 264873318 [](ancestors = 264873318,264872101,264822764)
| @@ -595,7 +595,7 @@ As a general rule, *if you use a parametric learner, you need to make sure your | |||
|
|
|||
| ML.NET offers several built-in scaling algorithms, or 'normalizers': | |||
| - MinMax normalizer: for each feature, we learn the minimum and maximum value of it, and then linearly rescale it so that the values fit between -1 and 1. | |||
| - MeanVar normalizer: for each feature, compute the mean and variance, and then linearly rescale it to zero-mean, unit-variance. | |||
| - MeanVariance normalizer: for each feature, compute the mean and variance, and then linearly rescale it to zero-mean, unit-variance. | |||
There was a problem hiding this comment.
MeanVariance [](start = 2, length = 12)
In the field, we usually use the term Standardize to reflect this normalization technique. This is very "statistics-y", but it does seem to be standard. How would everyone feel about changing "MeanVariance" to "Standardize", or at least offer a "Standardize" alias? #Resolved
There was a problem hiding this comment.
I don't feel MVN is super bad because it's already an operator in neural networks (e.g., ONNX, Caffe, CoreML).
Just for references:
https://apple.github.io/coremltools/coremlspecification/sections/NeuralNetwork.html#meanvariancenormalizelayerparams
https://github.com/onnx/onnx/blob/master/docs/Operators.md#MeanVarianceNormalization
In reply to: 264826182 [](ancestors = 264826182)
There was a problem hiding this comment.
That's a good point. Plus it's more precise than Standardize. Let's keep it MVN. We can always add an alias for Standardize if people are up in arms.
In reply to: 264891367 [](ancestors = 264891367,264826182)
| @@ -55,7 +55,7 @@ public static void Example() | |||
| //0.165 0.117 -0.547 0.014 | |||
|
|
|||
| // A pipeline to project Features column into L-p normalized vector. | |||
| var lpNormalizePipeline = ml.Transforms.LpNormalize(nameof(SamplesUtils.DatasetUtils.SampleVectorOfNumbersData.Features), normKind: Transforms.LpNormalizingEstimatorBase.NormFunction.L1); | |||
| var lpNormalizePipeline = ml.Transforms.LpNormalize(nameof(SamplesUtils.DatasetUtils.SampleVectorOfNumbersData.Features), norm: Transforms.LpNormalizingEstimatorBase.NormFunction.L1); | |||
There was a problem hiding this comment.
norm [](start = 134, length = 4)
We should be using long-form names here. #Resolved
There was a problem hiding this comment.
Ok. LpNormalize will be replaced by LpNormNormalize.
In reply to: 264826528 [](ancestors = 264826528)
| /// <remarks>Note that the statistics gathering and normalization is done independently per slot of the | ||
| /// vector values. | ||
| /// Note that if values are later transformed that are lower than the minimum, or higher than the maximum, | ||
| /// observed during fitting, that the output values may be outside the range of -1 to 1.</remarks> | ||
| /// <returns>The normalized column.</returns> | ||
| public static NormVector<float> Normalize( | ||
| this Vector<float> input, bool fixZero = FZ, long maxTrainingExamples = MaxTrain, | ||
| this Vector<float> input, bool ensureZeroUntouched = FZ, long maximumExampleCount = MaxTrain, |
There was a problem hiding this comment.
ensureZeroUntouched [](start = 43, length = 19)
it is still fixZero in the arguments. #Resolved
| @@ -1550,18 +1550,18 @@ public static Vector<float> FeaturizeText(this Scalar<string> input, Scalar<stri | |||
| } | |||
| } | |||
|
|
|||
| public static class RffStaticExtenensions | |||
| public static class RandomFourierKernelMappingStaticExtenensions | |||
There was a problem hiding this comment.
RandomFourierKernelMappingStaticExtenensions [](start = 24, length = 44)
we decided to go with acronyms #Resolved
There was a problem hiding this comment.
There was a problem hiding this comment.
Not anymore. Its acronyms will be AKM which is not a usual term. At least, it's my first time of seeing that.
In reply to: 264869094 [](ancestors = 264869094,264836908)
| /// <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> | ||
| /// <param name="generator">Which kernel to use. (if it is null, <see cref="GaussianKernel"/> is used.)</param> | ||
| /// <param name="seed">The seed of the random number generator for generating the new features. If not specified global random would be used.</param> | ||
| public static Vector<float> LowerVectorSizeWithRandomFourierTransformation(this Vector<float> input, | ||
| int dimension = RandomFourierKernelMappingEstimator.Defaults.Rank, bool useCosAndSinBases = RandomFourierKernelMappingEstimator.Defaults.UseCosAndSinBases, | ||
| public static Vector<float> RandomFourierKernelMapper(this Vector<float> input, |
There was a problem hiding this comment.
RandomFourierKernelMapper [](start = 36, length = 25)
shall we go with RfKernelMappingEstimator #Pending
There was a problem hiding this comment.
Discussed with Artidoro. We feel ApproximatedKernel is a better name for describing this transform's behavior.
In reply to: 264837298 [](ancestors = 264837298)
| } | ||
| } | ||
|
|
||
| public static class PcaEstimatorExtensions |
There was a problem hiding this comment.
PcaEstimatorExtensions [](start = 24, length = 22)
keeper #Resolved
There was a problem hiding this comment.
What? I guess you mean to have PcaStaticExtensions.
In reply to: 264837387 [](ancestors = 264837387)
There was a problem hiding this comment.
| { | ||
| public readonly bool FixZero; | ||
|
|
||
| private protected FixZeroColumnOptionsBase(string outputColumnName, string inputColumnName, long maxTrainingExamples, bool fixZero) | ||
| : base(outputColumnName, inputColumnName, maxTrainingExamples) | ||
| private protected ControlZeroColumnOptionsBase(string outputColumnName, string inputColumnName, long maximumExampleCount, bool fixZero) |
There was a problem hiding this comment.
ControlZeroColumnOptionsBase [](start = 30, length = 28)
Ditto above comment; I prefer FixZero. #Pending
There was a problem hiding this comment.
Zeros are not fixed. It floats around with different data sets. At my first glance of seeing FixZeroColumnOptionsBase, I expected that this column will fix my 0s as they are because otherwise it should be called FixOrChangeZeroColumnOptionsBase (or just ControlZeroColumnOptionsBase because it offers a special control over zeros).
In reply to: 264865051 [](ancestors = 264865051)
wschin
changed the title
| /// <param name="subMean">Subtract mean from each value before normalizing.</param> | ||
| public static Vector<float> LpNormalize(this Vector<float> input, | ||
| LpNormalizingEstimatorBase.NormFunction normKind = LpNormalizingEstimatorBase.Defaults.Norm, | ||
| bool subMean = LpNormalizingEstimatorBase.Defaults.LpEnsureZeroMean) => new OutPipelineColumn(input, normKind, subMean); | ||
| LpNormalizingEstimatorBase.NormFunction norm = LpNormalizingEstimatorBase.Defaults.Norm, |
There was a problem hiding this comment.
norm [](start = 52, length = 4)
Norm is not usually used as a stand-alone. Perhaps normalization? #Resolved
There was a problem hiding this comment.
Maybe no, how would user call ||x||_1? A norm I'd expect.
In reply to: 264866893 [](ancestors = 264866893)
There was a problem hiding this comment.
| /// <remarks>Note that the statistics gathering and normalization is done independently per slot of the | ||
| /// vector values. | ||
| /// Note that if values are later transformed that are lower than the minimum, or higher than the maximum, | ||
| /// observed during fitting, that the output values may be outside the range of -1 to 1.</remarks> | ||
| /// <returns>The normalized column.</returns> | ||
| public static NormVector<float> Normalize( | ||
| this Vector<float> input, bool fixZero = FZ, long maxTrainingExamples = MaxTrain, | ||
| this Vector<float> input, bool ensureZeroUntouched = FZ, long maximumExampleCount = MaxTrain, |
There was a problem hiding this comment.
ensureZeroUntouched [](start = 43, length = 19)
I'd prefer to be consistent throughout the APIs on what we call this. Right now, there are three names that this parameter goes by. #Resolved
There was a problem hiding this comment.
| @@ -16,61 +16,61 @@ namespace Microsoft.ML.StaticPipe | |||
| /// </summary> | |||
| public static class NormalizerStaticExtensions | |||
| { | |||
| private const long MaxTrain = NormalizingEstimator.Defaults.MaxTrainingExamples; | |||
| private const long MaxTrain = NormalizingEstimator.Defaults.MaximumExampleCount; | |||
| private const bool FZ = NormalizingEstimator.Defaults.FixZero; | |||
There was a problem hiding this comment.
Can we in-line these and just break lines in the code? #Resolved
There was a problem hiding this comment.
| OnFitAffine<ImmutableArray<float>> onFit = null) | ||
| { | ||
| return NormalizeByMVCdfCore(input, fixZero, useLog, false, maxTrainingExamples, AffineMapper(onFit)); | ||
| return NormalizeByMVCdfCore(input, ensureZeroUntouched, useLog, false, maximumExampleCount, AffineMapper(onFit)); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Learns an affine function based on the observed mean and standard deviation. This is less susceptible | ||
| /// to outliers as compared to <see cref="Normalize(Vector{double}, bool, long, OnFitAffine{ImmutableArray{double}})"/>. | ||
| /// </summary> |
There was a problem hiding this comment.
This should be expanded upon at some time. Maybe in the docs go-around. #WontFix
There was a problem hiding this comment.
| @@ -71,15 +71,15 @@ public static class NormalizationCatalog | |||
| /// </format> | |||
| /// </example> | |||
| public static LpNormalizingEstimator LpNormalize(this TransformsCatalog catalog, string outputColumnName, string inputColumnName = null, | |||
There was a problem hiding this comment.
This name isn't the best.... #Resolved
There was a problem hiding this comment.
Don't forget to add an alias for the old name when you change the name. You will also need to update the [arguments] in the original class to have this alias as well. #Resolved Refers to: test/BaselineOutput/Common/EntryPoints/core_manifest.json:16559 in ed6e8b3. [](commit_id = ed6e8b3, deletion_comment = False) |
| @@ -383,7 +383,9 @@ public ColumnFunctionAccessor(ImmutableArray<ColumnOptions> infos) | |||
| [BestFriend] | |||
| internal readonly IReadOnlyList<IColumnFunction> ColumnFunctions; | |||
|
|
|||
| public readonly ImmutableArray<ColumnOptions> Columns; | |||
| [BestFriend] | |||
| internal readonly ImmutableArray<ColumnOptions> Columns; | |||
There was a problem hiding this comment.
ImmutableArray [](start = 26, length = 14)
@rogancarr I've ask @wschin to hide this right now, for a reason we don't have common idea of how to do it. Hope you Ok with that. We can always expose it in v1.1
#Resolved
There was a problem hiding this comment.
we need to leave this public for this transform, as there is a user use-case.
Or we need to come up with another way to expose the model parameters for the normalizer, but we cannot hide them.
In reply to: 264910410 [](ancestors = 264910410)
There was a problem hiding this comment.
There was a problem hiding this comment.
If you can add a comment about not changing this back to internal, as it has customer usage.
I'll add a functional test about it
In reply to: 265317859 [](ancestors = 265317859,265306118,264910410)
There was a problem hiding this comment.
It has a test added by Rogan.
In reply to: 265323244 [](ancestors = 265323244,265317859,265306118,264910410)
| MinMaxNormalized: r.Features.Normalize(fixZero: true), | ||
| MeanVarNormalized: r.Features.NormalizeByMeanVar(fixZero: false), | ||
| MinMaxNormalized: r.Features.Normalize(ensureZeroUntouched: true), | ||
| MeanVarNormalized: r.Features.NormalizeByMeanVariance(ensureZeroUntouched: false), |
There was a problem hiding this comment.
ensureZeroUntouched [](start = 74, length = 19)
nit: ensureZeroUnchanged #WontFix
There was a problem hiding this comment.
I like the word touched, because it makes feel that Normalizer is a cute animal which sometime plays with zeros.
In reply to: 265304319 [](ancestors = 265304319)
| bool subMean = LpNormalizingEstimatorBase.Defaults.LpEnsureZeroMean) => new OutPipelineColumn(input, normKind, subMean); | ||
| /// <param name="norm">Type of norm to use to normalize each sample.</param> | ||
| /// <param name="ensureZeroMean">Subtract mean from each value before normalizing.</param> | ||
| public static Vector<float> LpNormNormalize(this Vector<float> input, |
There was a problem hiding this comment.
LpNormNormalize [](start = 36, length = 15)
is it too bad to keep it as is? NormNormalizing feels like a tongue twist :) #Pending
There was a problem hiding this comment.
That was suggested by Rogan for alignment with other normalizations. Let me try NormalizeLpNorm.
In reply to: 265323674 [](ancestors = 265323674)
There was a problem hiding this comment.
There was a problem hiding this comment.
LpNormalizer normalizes a vector in a column and not columns in an IDataView, so maybe we should just call it NormalizeVector.
In reply to: 265328368 [](ancestors = 265328368,265327419,265323674)
There was a problem hiding this comment.
There are several normalizers around. We can't drop things like LpNorm and GlobalContrast.
In reply to: 265340604 [](ancestors = 265340604,265328368,265327419,265323674)
| @@ -47,16 +47,16 @@ public Reconciler(LpNormalizingEstimatorBase.NormFunction normKind, bool ensureZ | |||
| foreach (var outCol in toOutput) | |||
| pairs.Add((outputNames[outCol], inputNames[((OutPipelineColumn)outCol).Input])); | |||
|
|
|||
| return new LpNormalizingEstimator(env, pairs.ToArray(), _norm, _ensureZeroMean); | |||
| return new LpNormNormalizingEstimator(env, pairs.ToArray(), _norm, _ensureZeroMean); | |||
| } | |||
| } | |||
|
|
|||
| /// <include file='../Microsoft.ML.Transforms/doc.xml' path='doc/members/member[@name="LpNormalize"]/*'/> | |||
| /// <param name="input">The column to apply to.</param> | |||
There was a problem hiding this comment.
The column to apply to. [](start = 32, length = 23)
Isn't this a row? Oh, I see. This should say something like "The column containing the vectors to apply the normalization to." #Resolved
There was a problem hiding this comment.
| @@ -384,7 +384,11 @@ public ColumnFunctionAccessor(ImmutableArray<ColumnOptions> infos) | |||
| [BestFriend] | |||
| internal readonly IReadOnlyList<IColumnFunction> ColumnFunctions; | |||
|
|
|||
| /// <summary> | |||
| /// The configuration of the normalizer. The i-th element describes the i-th input-output column pair. | |||
| /// </summary> | |||
| public readonly ImmutableArray<ColumnOptions> Columns; | |||
There was a problem hiding this comment.
ColumnOptions [](start = 39, length = 13)
Since you are doing the Normalizer scrubbing, we need to rename ColumnOptions here. It really does not make sense for it to be named like this.
#2854 #Pending
There was a problem hiding this comment.
It doesn't look like that thread converges already..
In reply to: 265349922 [](ancestors = 265349922)
ghost
locked as resolved and limited conversation to collaborators
Follow up of #2865 so should fix #2928. Some public APIs were missed.