Skip to content

XML doc for LpNormalizingEstimator and GlobalContrastNormalizingEstimator #3454

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 2 commits into from
Apr 22, 2019

Conversation

artidoro
Copy link
Contributor

Tracked by #3204 .

This PR adds xml doc for LpNormalizingEstimator and GlobalContrastNormalizingEstimator.

@artidoro artidoro added the documentation Related to documentation of ML.NET label Apr 21, 2019
@artidoro artidoro requested review from natke and shmoradims April 21, 2019 22:33
@artidoro artidoro self-assigned this Apr 21, 2019
/// Let $\mu(x) = \sum_i x_i / n$ be the mean of the values of vector $x$. The <xref:Microsoft.ML.Transforms.LpNormNormalizingTransformer>
/// performs the following operation on each input vector $x$:
///
/// $y = \frac{x - \mu(x)}{L(x)}$
Copy link
Contributor Author

@artidoro artidoro Apr 21, 2019

Choose a reason for hiding this comment

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

I am not sure if I should use the double dollar sign here? #Resolved

Copy link
Member

@najeeb-kazmi najeeb-kazmi Apr 21, 2019

Choose a reason for hiding this comment

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

This looks fine to me, but the following may be better:

\begin{equation*}
<equation without $ signs>
\end{equation*}

P.S. Don't use $$ as it is plain TeX and may not render in LaTex


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

Copy link
Member

Choose a reason for hiding this comment

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

Same for the equations for the other norm funstions


In reply to: 277185443 [](ancestors = 277185443,277183775)

Copy link
Member

@najeeb-kazmi najeeb-kazmi Apr 21, 2019

Choose a reason for hiding this comment

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

I think the \begin{equation*} would need the amsmath package available wherever the LaTeX is being rendered so maybe use \[ and \] around the equation instead (only if you decide to replace $, which I don't think is necessary). #Resolved

Copy link
Contributor

@natke natke Apr 21, 2019

Choose a reason for hiding this comment

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

This is in a markdown block, so I think you need the $ $ #Resolved

Copy link
Contributor Author

@artidoro artidoro Apr 22, 2019

Choose a reason for hiding this comment

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

But single($equation$) or double ($$equation$$)?

I would like those to be centered. But it is a detail. #Resolved

@codecov
Copy link

codecov bot commented Apr 21, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@7ca768a). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #3454   +/-   ##
=========================================
  Coverage          ?   72.77%           
=========================================
  Files             ?      808           
  Lines             ?   145452           
  Branches          ?    16244           
=========================================
  Hits              ?   105847           
  Misses            ?    35186           
  Partials          ?     4419
Flag Coverage Δ
#Debug 72.77% <ø> (?)
#production 68.28% <ø> (?)
#test 89.04% <ø> (?)
Impacted Files Coverage Δ
src/Microsoft.ML.Transforms/NormalizerCatalog.cs 84.78% <ø> (ø)
src/Microsoft.ML.Transforms/GcnTransform.cs 77.57% <ø> (ø)

/// The data type on this column is the same as the input column.</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>
/// This column's data type will be the same as the input column's data type.</param>
/// <param name="inputColumnName">Name of column to transform. If set to <see langword="null"/>, the value of the
Copy link
Member

@najeeb-kazmi najeeb-kazmi Apr 21, 2019

Choose a reason for hiding this comment

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

Name of column to transform [](start = 42, length = 27)

Name of the column to normalize, but if you're nopt changing this for the remaining normalizers (not included in this PR), I'd rather keep this consistent and leave it be. #Resolved

/// This column's data type will be the same as the input column's data type.</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.
/// This estimator operates over known-sized vectors of <see cref="System.Single"/>.</param>
/// <param name="norm">Type of norm to use to normalize each sample. The indicated norm of the resulted vector will be normalized to one.</param>
Copy link
Member

@najeeb-kazmi najeeb-kazmi Apr 21, 2019

Choose a reason for hiding this comment

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

resulted [](start = 103, length = 8)

resulting #Resolved

/// The data type on this column is the same as the input column.</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>
/// This column's data type will be the same as the input column's data type.</param>
/// <param name="inputColumnName">Name of column to transform. If set to <see langword="null"/>, the value of the
Copy link
Member

@najeeb-kazmi najeeb-kazmi Apr 21, 2019

Choose a reason for hiding this comment

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

Name of column to transform [](start = 42, length = 27)

Same as above #Resolved

/// <param name="ensureZeroMean">If <see langword="true"/>, subtract mean from each value before normalizing and use the raw input otherwise.</param>
/// <param name="ensureUnitStandardDeviation">If <see langword="true"/>, resulted vector's standard deviation would be one. Otherwise, resulted vector's L2-norm would be one.</param>
/// <param name="ensureUnitStandardDeviation">If <see langword="true"/>, resulted vector's standard deviation would be one.
Copy link
Member

@najeeb-kazmi najeeb-kazmi Apr 21, 2019

Choose a reason for hiding this comment

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

resulting #Resolved

///
///
/// The resulting <xref:Microsoft.ML.Transforms.LpNormNormalizingTransformer> normalizes vectors in the input column individually
/// by rescaling them applying the global contrast normalization. The <xref:Microsoft.ML.Transforms.LpNormNormalizingTransformer>
Copy link
Member

@najeeb-kazmi najeeb-kazmi Apr 21, 2019

Choose a reason for hiding this comment

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

maybe "...individually, rescaling them by applying global contrast normalization" #Resolved

Copy link
Member

@najeeb-kazmi najeeb-kazmi left a comment

Choose a reason for hiding this comment

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

:shipit:

@najeeb-kazmi
Copy link
Member

najeeb-kazmi commented Apr 21, 2019

Please see minor comments #Resolved

/// | Output column data type | Vector of <xref:System.Single> |
///
///
/// The resulting <xref:Microsoft.ML.Transforms.LpNormNormalizingTransformer> normalizes vectors in the input column individually
Copy link
Contributor

@natke natke Apr 21, 2019

Choose a reason for hiding this comment

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

Should this be GlobalContrastNormalizingEstimator? #ByDesign

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both the GlobalContrastNormalizingEstimator and the LpNormalizingEstimator lead to the same transformer the LpNormalizingTransformer after training


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

/// $y = \frac{x}{L(x)}$
///
/// There are four types of norm that can be selected by the user to be applied on input vector $x$. They are defined as follows:
/// - <xref:Microsoft.ML.Transforms.Text.TextFeaturizingEstimator.NormFunction.L1>
Copy link
Contributor

@natke natke Apr 22, 2019

Choose a reason for hiding this comment

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

Should these be the Microsoft.ML.Transforms.LpNormNormalizingEstimatorBase.NormFunction values rather than the TextFeaturizing ones? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you are right! Thank you for the catch!!


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

@artidoro artidoro merged commit dc61418 into dotnet:master Apr 22, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Related to documentation of ML.NET
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants