Update xml documentation for Image estimators #3376

Ivanidzo4ka · 2019-04-17T18:07:51Z

towards #3204

codecov · 2019-04-17T18:44:54Z

Codecov Report

Merging #3376 into master will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3376      +/-   ##
==========================================
+ Coverage   72.69%   72.76%   +0.06%     
==========================================
  Files         807      808       +1     
  Lines      145171   145452     +281     
  Branches    16225    16244      +19     
==========================================
+ Hits       105536   105839     +303     
+ Misses      35220    35192      -28     
- Partials     4415     4421       +6

Flag	Coverage Δ
#Debug	`72.76% <ø> (+0.06%)`	⬆️
#production	`68.27% <ø> (+0.04%)`	⬆️
#test	`89.04% <ø> (+0.06%)`	⬆️

Impacted Files	Coverage Δ
src/Microsoft.ML.ImageAnalytics/ImageLoader.cs	`84.55% <ø> (ø)`	⬆️
src/Microsoft.ML.ImageAnalytics/ImageGrayscale.cs	`78.33% <ø> (ø)`	⬆️
...rosoft.ML.ImageAnalytics/VectorToImageTransform.cs	`76.77% <ø> (ø)`	⬆️
...Microsoft.ML.ImageAnalytics/ImagePixelExtractor.cs	`82.55% <ø> (ø)`	⬆️
src/Microsoft.ML.ImageAnalytics/ImageResizer.cs	`84.64% <ø> (ø)`	⬆️
...c/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs	`16.66% <ø> (ø)`	⬆️
src/Microsoft.ML.Data/Transforms/KeyToValue.cs	`79.16% <0%> (-0.65%)`	⬇️
src/Microsoft.ML.Transforms/Text/LdaTransform.cs	`89.26% <0%> (-0.63%)`	⬇️
...ML.Transforms/MutualInformationFeatureSelection.cs	`78.58% <0%> (-0.33%)`	⬇️
...ft.ML.Data/Evaluators/BinaryClassifierEvaluator.cs	`77.18% <0%> (-0.01%)`	⬇️
... and 64 more

natke

A few minor comments.

natke · 2019-04-18T19:29:14Z

src/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs

@@ -23,9 +28,12 @@ public static class ImageEstimatorsCatalog
        public static ImageGrayscalingEstimator ConvertToGrayscale(this TransformsCatalog catalog, string outputColumnName, string inputColumnName = null)
            => new ImageGrayscalingEstimator(CatalogUtils.GetEnvironment(catalog), new[] { (outputColumnName, inputColumnName ?? outputColumnName) });

-        /// <include file='doc.xml' path='doc/members/member[@name="ImageGrayscalingEstimator"]/*' />
+        ///<summary>
+        /// Create a <see cref="ImageGrayscalingEstimator"/>, which converts images in the column specified in <see cref="InputOutputColumnPair.InputColumnName"/>


Should we distinguish between the two methods? i.e. one uses the column names directly, and one supplies a column pair #ByDesign

Second method is internal, so main reason why I'm even adding anything here is because I want to get rid of doc.xml.

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

natke · 2019-04-18T19:29:50Z

src/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs

        /// <param name="catalog">The transform's catalog.</param>
-        /// <param name="columns">Specifies the names of the input columns for the transformation, and their respective output column names.</param>
+        /// <param name="columns">The pairs of input and output columns. This estimator operates over any data type.</param>


Is that true? Doesn't the input have to be image data? #Resolved

Update to proper description

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

natke · 2019-04-18T19:30:11Z

src/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs

@@ -41,20 +49,15 @@ internal static ImageGrayscalingEstimator ConvertToGrayscale(this TransformsCata
        }

        /// <summary>
-        /// Loads the images from the <see cref="ImageLoadingTransformer.ImageFolder" /> into memory.
+        /// Create a <see cref="ImageLoadingEstimator"/>, which load the data from the column specified in <paramref name="inputColumnName"/>


load --> loads #Resolved

sfilipi · 2019-04-19T05:43:18Z

src/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs

+        /// <param name="outputColumnName">Name of the column resulting from the transformation of <paramref name="inputColumnName"/>.
+        /// This column's data type will be <see cref="System.Drawing.Bitmap"/>.</param>
+        /// <param name="inputColumnName">Name of the column to copy the data from.
+        /// This estimator operates over text data.</param>


over text data [](start = 36, length = 14)

over text data indicating the path of the images to load. #Resolved

I rather put it in sentence above.

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

sfilipi · 2019-04-19T05:44:09Z

    public static ImageLoadingEstimator LoadImages(this TransformsCatalog catalog, string outputColumnName, string imageFolder, string inputColumnName = null)

idk why TextLoader is its own privileged type, but LoadImages is an estimator.. #endRant #WontFix

Refers to: src/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs:67 in 6d4e789. [](commit_id = 6d4e789, deletion_comment = False)

sfilipi · 2019-04-19T05:44:48Z

src/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.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="outputColumnName">Name of the column resulting from the transformation of <paramref name="inputColumnName"/>.
+        /// This column's data type will be known size vector of <see cref="System.Single"/>or <see cref="System.Byte"/> depending on <paramref name="outputAsFloatArray"/>.</param>


e known [](start = 42, length = 7)

be a known #Resolved

sfilipi · 2019-04-19T05:44:58Z

src/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.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="outputColumnName">Name of the column resulting from the transformation of <paramref name="inputColumnName"/>.
+        /// This column's data type will be known size vector of <see cref="System.Single"/>or <see cref="System.Byte"/> depending on <paramref name="outputAsFloatArray"/>.</param>


o [](start = 92, length = 1)

space #Resolved

sfilipi · 2019-04-19T05:45:34Z

src/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs

-        /// <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="outputColumnName">Name of the column resulting from the transformation of <paramref name="inputColumnName"/>.
+        /// This column's data type will be known size vector of <see cref="System.Single"/>or <see cref="System.Byte"/> depending on <paramref name="outputAsFloatArray"/>.</param>
+        /// <param name="inputColumnName">Name of the column to copy the data from.


Name of the column to copy the data from [](start = 42, length = 40)

nope :) #Resolved

sfilipi · 2019-04-19T05:45:55Z

src/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs

-        /// <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="outputColumnName">Name of the column resulting from the transformation of <paramref name="inputColumnName"/>.
+        /// This column's data type will be the same as that of the input column.</param>
+        /// <param name="inputColumnName">Name of the column to copy the data from.


Name of the column to copy the data from. [](start = 42, length = 41)

fix me :) #Resolved

I can do what, but who gonna fix me?

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

sfilipi · 2019-04-19T05:46:27Z

src/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs

-        /// <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="outputColumnName">Name of the column resulting from the transformation of <paramref name="inputColumnName"/>.
+        /// This column's data type will be <see cref="System.Drawing.Bitmap"/>.</param>
+        /// <param name="inputColumnName">Name of the column to copy the data from.


Name of the column to copy the data from. [](start = 42, length = 41)

fix me too. #Resolved

sfilipi · 2019-04-19T05:47:06Z

    /// <seealso cref= "ImageLoadingEstimator" />

please get those outside of , since you're at it.. #Resolved

Refers to: src/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs:180 in 6d4e789. [](commit_id = 6d4e789, deletion_comment = False)

sfilipi · 2019-04-19T05:47:40Z

src/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs

@@ -76,7 +79,7 @@ public static ImageLoadingEstimator LoadImages(this TransformsCatalog catalog, s
        /// <seealso cref = "ImageEstimatorsCatalog" />


/// [](start = 8, length = 47)

please remove #Resolved

it's on top of internal method, but will do.

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

sfilipi · 2019-04-19T05:48:04Z

src/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs

-        /// <param name="inputColumnName">Name of the column to transform. If set to <see langword="null"/>, the value of the <paramref name="outputColumnName"/> will be used as source.</param>
+        /// <param name="outputColumnName">Name of the column resulting from the transformation of <paramref name="inputColumnName"/>.
+        /// This column's data type will be <see cref="System.Drawing.Bitmap"/>.</param>
+        /// <param name="inputColumnName">Name of the column to copy the data from.


Name of the column to copy the data from. [](start = 42, length = 41)

fix #Resolved

sfilipi · 2019-04-19T05:48:21Z

src/Microsoft.ML.ImageAnalytics/ImageGrayscale.cs

@@ -32,18 +32,8 @@ namespace Microsoft.ML.Transforms.Image
    // REVIEW: Rewrite as LambdaTransform to simplify.
    // REVIEW: Should it be separate transform or part of ImageResizerTransform?
    /// <summary>
-    /// <see cref="ITransformer"/> produced by fitting the <see cref="IDataView"/> to an <see cref="ImageGrayscalingEstimator" />.
+    /// <see cref="ITransformer"/> resulting from fitting a <see cref="ImageGrayscalingTransformer"/>.


a [](start = 58, length = 1)

a --> an #Resolved

I think in past month I learn more about English grammar rather than machine learning or C# development.
Not sure what to think about this.

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

sfilipi · 2019-04-19T05:49:15Z

src/Microsoft.ML.ImageAnalytics/ImageGrayscale.cs

+    /// | Output column data type | <xref:System.Drawing.Bitmap> |
+    ///
+    /// The resulting <xref:Microsoft.ML.Transforms.Image.ImageGrayscalingTransformer> creates a new column, named as specified in the output column name parameters, and
+    /// converts image from input column into grayscale image.


image [](start = 16, length = 7)

the image from the input column into a grayscale image. #Resolved

sfilipi · 2019-04-19T05:50:03Z

src/Microsoft.ML.ImageAnalytics/ImageGrayscale.cs

-    /// <seealso cref = "ImageEstimatorsCatalog" />
-    /// </remarks >
+    /// [examples](https://github.com/dotnet/machinelearning-samples/tree/master/samples/csharp/getting-started) in the machinelearning-samples github repository.
+    /// See the See Also section for links to more examples of the usage.


examples of the usage [](start = 51, length = 21)

@natke usage examples?

Yes, that sounds better

sfilipi · 2019-04-19T05:50:19Z

src/Microsoft.ML.ImageAnalytics/ImageLoader.cs

@@ -29,16 +29,8 @@
 namespace Microsoft.ML.Data
 {
    /// <summary>
-    /// The <see cref="ITransformer"/> produced by fitting an <see cref="IDataView"/> to an <see cref="ImageLoadingEstimator"/>.
+    /// <see cref="ITransformer"/> resulting from fitting a <see cref="ImageLoadingEstimator"/>.


a [](start = 58, length = 1)

an #Resolved

natke · 2019-04-20T17:11:07Z

src/Microsoft.ML.ImageAnalytics/ImageGrayscale.cs

-    /// <seealso cref = "ImageEstimatorsCatalog" />
-    /// </remarks >
+    /// [examples](https://github.com/dotnet/machinelearning-samples/tree/master/samples/csharp/getting-started) in the machinelearning-samples github repository.
+    /// See the See Also section for links to more examples of the usage.


Yes, that sounds better

natke · 2019-04-20T17:11:47Z

src/Microsoft.ML.ImageAnalytics/ImageResizer.cs

+    /// | Output column data type | <xref:System.Drawing.Bitmap> |
+    ///
+    /// The resulting <xref:Microsoft.ML.Transforms.Image.ImageResizingTransformer> creates a new column, named as specified in the output column name parameters, and
+    /// resize the data from the input column to this new column.


resize -> resizes #Resolved

shmoradims · 2019-04-20T18:08:29Z

src/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.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="outputColumnName">Name of the column resulting from the transformation of <paramref name="inputColumnName"/>.
+        /// This column's data type will be a known-size vector of <see cref="System.Single"/> or <see cref="System.Byte"/> depending on <paramref name="outputAsFloatArray"/>.</param>


size [](start = 52, length = 4)

sized #Resolved

shmoradims · 2019-04-20T18:09:53Z

    /// <seealso cref= "ImageLoadingEstimator" />

don't worry about 'internal' docs at this point

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

Refers to: src/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs:180 in 6d4e789. [](commit_id = 6d4e789, deletion_comment = False)

shmoradims · 2019-04-20T18:11:40Z

src/Microsoft.ML.ImageAnalytics/ImagePixelExtractor.cs

+    /// | -- | -- |
+    /// | Does this estimator need to look at the data to train its parameters? | No |
+    /// | Input column data type | <xref:System.Drawing.Bitmap> |
+    /// | Output column data type | Vector of known size of <xref:System.Single> or <xref:System.Byte> |


Vector of known size [](start = 36, length = 20)

Known-sized vector of ...

shmoradims · 2019-04-20T18:11:59Z

src/Microsoft.ML.ImageAnalytics/ImagePixelExtractor.cs

+    /// | Output column data type | Vector of known size of <xref:System.Single> or <xref:System.Byte> |
+    ///
+    /// The resulting <xref:Microsoft.ML.Transforms.Image.ImagePixelExtractingTransformer> creates a new column, named as specified in the output column name parameters, and
+    /// converts image into vector of known size of floats or bytes. Size and data type depends on specified paramaters.


vector of known size of [](start = 28, length = 23)

ditto

shmoradims · 2019-04-20T18:24:38Z

src/Microsoft.ML.ImageAnalytics/ImagePixelExtractor.cs

+    /// | Does this estimator need to look at the data to train its parameters? | No |
+    /// | Input column data type | <xref:System.Drawing.Bitmap> |
+    /// | Output column data type | Vector of known size of <xref:System.Single> or <xref:System.Byte> |
+    ///


/// [](start = 2, length = 5)

please add: |Required NuGet in addition to Microsoft.ML| the nuget name |

also to other image transforms in the PR #Resolved

shmoradims · 2019-04-20T18:26:14Z

src/Microsoft.ML.ImageAnalytics/VectorToImageTransform.cs

+    /// |  |  |
+    /// | -- | -- |
+    /// | Does this estimator need to look at the data to train its parameters? | No |
+    /// | Input column data type | Vector of known size of floats, doubles or bytes. |


floats, doubles or bytes [](start = 59, length = 24)

use system types please, for the sake of F# users #Resolved

shmoradims

Update xml documentation for Image estimators

bb3ed19

Ivanidzo4ka requested review from shmoradims, natke and sfilipi April 17, 2019 18:08

Remove doc.xml

e2189f3

update markdown

5157a9d

sfilipi mentioned this pull request Apr 17, 2019

API reference - XML documentation template for transforms #3204

Closed

Merge branch 'master' into Ivanidze/ImagesXmlDocumentationPassOne

74dda82

natke suggested changes Apr 18, 2019

View reviewed changes

Ivan Matantsev added 2 commits April 18, 2019 14:01

update to different types

1ed6278

adress comments

6d4e789

Ivanidzo4ka added the documentation Related to documentation of ML.NET label Apr 19, 2019

sfilipi reviewed Apr 19, 2019

View reviewed changes

Ivan Matantsev added 2 commits April 18, 2019 23:05

address senja comments

911e373

ReadOnly.. to TextDataViewType

7d1720b

Ivanidzo4ka requested a review from natke April 19, 2019 23:16

Ivanidzo4ka requested review from sfilipi and removed request for natke April 19, 2019 23:16

natke reviewed Apr 20, 2019

View reviewed changes

natke self-requested a review April 20, 2019 17:12

natke approved these changes Apr 20, 2019

View reviewed changes

shmoradims reviewed Apr 20, 2019

View reviewed changes

shmoradims approved these changes Apr 20, 2019

View reviewed changes

address comments

c060327

shmoradims merged commit 4deb4fc into dotnet:master Apr 21, 2019

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

		@@ -76,7 +79,7 @@ public static ImageLoadingEstimator LoadImages(this TransformsCatalog catalog, s
		/// <seealso cref = "ImageEstimatorsCatalog" />

Update xml documentation for Image estimators #3376

Update xml documentation for Image estimators #3376

Conversation

Ivanidzo4ka commented Apr 17, 2019

codecov bot commented Apr 17, 2019 • edited Loading

Codecov Report

natke left a comment

Choose a reason for hiding this comment

natke Apr 18, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natke Apr 18, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natke Apr 18, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

sfilipi Apr 19, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi commented Apr 19, 2019 • edited Loading

sfilipi Apr 19, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

sfilipi Apr 19, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

sfilipi Apr 19, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

sfilipi Apr 19, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Apr 19, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

sfilipi commented Apr 19, 2019 • edited by Ivanidzo4ka Loading

sfilipi Apr 19, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Apr 19, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

sfilipi Apr 19, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Apr 19, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Apr 19, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natke Apr 20, 2019 • edited by shmoradims Loading

Choose a reason for hiding this comment

shmoradims Apr 20, 2019 • edited Loading

Choose a reason for hiding this comment

shmoradims commented Apr 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shmoradims Apr 20, 2019 • edited Loading

Choose a reason for hiding this comment

shmoradims Apr 20, 2019 • edited Loading

Choose a reason for hiding this comment

shmoradims left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 17, 2019 •

edited

Loading

natke Apr 18, 2019 •

edited by Ivanidzo4ka

Loading

natke Apr 18, 2019 •

edited by Ivanidzo4ka

Loading

natke Apr 18, 2019 •

edited by Ivanidzo4ka

Loading

sfilipi Apr 19, 2019 •

edited by Ivanidzo4ka

Loading

sfilipi commented Apr 19, 2019 •

edited

Loading

sfilipi Apr 19, 2019 •

edited by Ivanidzo4ka

Loading

sfilipi Apr 19, 2019 •

edited by Ivanidzo4ka

Loading

sfilipi Apr 19, 2019 •

edited by Ivanidzo4ka

Loading

sfilipi Apr 19, 2019 •

edited by Ivanidzo4ka

Loading

sfilipi Apr 19, 2019 •

edited by Ivanidzo4ka

Loading

sfilipi commented Apr 19, 2019 •

edited by Ivanidzo4ka

Loading

sfilipi Apr 19, 2019 •

edited by Ivanidzo4ka

Loading

sfilipi Apr 19, 2019 •

edited by Ivanidzo4ka

Loading

sfilipi Apr 19, 2019 •

edited by Ivanidzo4ka

Loading

sfilipi Apr 19, 2019 •

edited by Ivanidzo4ka

Loading

sfilipi Apr 19, 2019 •

edited by Ivanidzo4ka

Loading

natke Apr 20, 2019 •

edited by shmoradims

Loading

shmoradims Apr 20, 2019 •

edited

Loading

shmoradims Apr 20, 2019 •

edited

Loading

shmoradims Apr 20, 2019 •

edited

Loading