VectorToImageTransform conversion to estimator/transformer #2580

yaeldekel · 2019-02-15T22:17:34Z

This is the remaining work for the public API of Microsoft.ML.ImageAnalytics.
Fixes #2269 .

TomFinley · 2019-02-19T16:42:12Z

src/Microsoft.ML.ImageAnalytics/VectorToImageTransform.cs

+                if (!inputSchema.TryFindColumn(colInfo.InputColumnName, out var col))
+                    throw Host.ExceptSchemaMismatch(nameof(inputSchema), "input", colInfo.InputColumnName);
+                if (col.Kind != SchemaShape.Column.VectorKind.Vector || (col.ItemType != NumberDataViewType.Single && col.ItemType != NumberDataViewType.Double && col.ItemType != NumberDataViewType.Byte))
+                    throw Host.ExceptSchemaMismatch(nameof(inputSchema), "input", colInfo.InputColumnName, new ImageType().ToString(), col.GetTypeString());


new ImageType().ToString() [](start = 107, length = 26)

This is not correct. You're saying the expected input type is an image type, yet, you are testing whether it is a known-size vector of float, double, or byte above. #Resolved

TomFinley · 2019-02-19T16:42:44Z

src/Microsoft.ML.ImageAnalytics/VectorToImageTransform.cs

-            public bool Red { get { return (Colors & ColorBits.Red) != 0; } }
-            public bool Green { get { return (Colors & ColorBits.Green) != 0; } }
-            public bool Blue { get { return (Colors & ColorBits.Blue) != 0; } }
+            public bool Alpha { get { return (Colors & ImagePixelExtractingEstimator.ColorBits.Alpha) != 0; } }


public bool Alpha { get { return (Colors & ImagePixelExtractingEstimator.ColorBits.Alpha) != 0; } } [](start = 12, length = 99)

Not a big deal, but if you wanted to bring this transplanted code up to "modern standards" by using => style syntax to make this a bit less crazy taht would be fine with me. #Resolved

Ivanidzo4ka · 2019-02-19T17:46:01Z

src/Microsoft.ML.ImageAnalytics/VectorToImageTransform.cs

+                    if (sourceItemType == NumberDataViewType.Byte)
+                    return GetterFromType<byte>(input, iinfo, ex, false);
+                else
+                    throw Contracts.Except("We only support float or byte arrays");


We only support float or byte arrays [](start = 44, length = 36)

From estimator: "known-size vector of type float, double or byte
Double get lost. #Resolved

Ivanidzo4ka · 2019-02-19T17:50:47Z

test/Microsoft.ML.Tests/ImagesTests.cs

@@ -201,13 +201,13 @@ public void TestBackAndForthConversionWithAlphaInterleave()
            var cropped = new ImageResizingTransformer(env, "ImageCropped", imageWidth, imageHeight, "ImageReal").Transform(images);

            var pixels = new ImagePixelExtractingTransformer(env, "ImagePixels", "ImageCropped", ImagePixelExtractingEstimator.ColorBits.All, true, 2f / 255, 127.5f).Transform(cropped);
-            IDataView backToBitmaps = new VectorToImageTransform(env, new VectorToImageTransform.Arguments()
+            IDataView backToBitmaps = VectorToImageConvertingTransformer.Create(env, new VectorToImageConvertingTransformer.Options()


VectorToImageConvertingTransformer.Create [](start = 38, length = 41)

Please don't do that.

First it's a different style comparing to all other transformers.

Second Create method is IDataTransform which we want to get rid of. #Resolved

Ivanidzo4ka · 2019-02-19T17:52:58Z

src/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs

+        /// <param name="scale">The values are scaled by this value before being converted to pixels.</param>
+        /// <param name="offset">The offset is subtracted (before scaling) before converting the values to pixels.</param>
+        public static VectorToImageConvertingEstimator ConvertToImage(this TransformsCatalog catalog, int height, int width, string outputColumnName, string inputColumnName = null,
+            ImagePixelExtractingEstimator.ColorBits colors = ImagePixelExtractingEstimator.ColorBits.Rgb, bool interleave = false, float scale = 1, float offset = 0)


1 [](start = 145, length = 1)

would be nice to have Defaults class #Resolved

codecov · 2019-02-19T18:24:49Z

Codecov Report

Merging #2580 into master will decrease coverage by <.01%.
The diff coverage is 77.9%.

@@            Coverage Diff             @@
##           master    #2580      +/-   ##
==========================================
- Coverage    71.5%   71.49%   -0.01%     
==========================================
  Files         801      800       -1     
  Lines      142023   141899     -124     
  Branches    16147    16133      -14     
==========================================
- Hits       101557   101455     -102     
  Misses      35998    35998              
+ Partials     4468     4446      -22

Flag	Coverage Δ
#Debug	`71.49% <77.9%> (-0.01%)`	⬇️
#production	`67.78% <76.44%> (-0.01%)`	⬇️
#test	`85.75% <100%> (+0.17%)`	⬆️

Ivanidzo4ka · 2019-02-20T00:27:48Z

src/Microsoft.ML.ImageAnalytics/VectorToImageTransform.cs

+                //verWrittenCur: 0x00010001, // Initial
+                //verWrittenCur: 0x00010002, // Swith from OpenCV to Bitmap
+                verWrittenCur: 0x00010003, // don't serialize sizeof(Single)
+                verReadableCur: 0x00010003,


0x00010003 [](start = 32, length = 10)

You can always have check for model version and read one Int32 from stream if it's old one.

I have separate PR where I bump version for ImagePixelExtractor (I introduce colors order). So I plan do the same for VectorToImage and bump version again.
I can get rid of single and add order in same time.
Does it sound good for you?
If you still want to get rid of single in this iteration, I would suggest to keep old models readable.

Ivanidzo4ka

One small comment regarding VersionInfo.
Otherwise looks good.

zeahmed · 2019-02-20T23:07:42Z

src/Microsoft.ML.ImageAnalytics/VectorToImageTransform.cs

        /// </summary>
-        [Flags]
-        private enum ColorBits : byte
+        public IReadOnlyCollection<VectorToImageConvertingEstimator.ColumnInfo> Columns => _columns.AsReadOnly();


public [](start = 8, length = 6)

internal???

zeahmed

TomFinley

Thanks @yaeldekel ! In an ideal world we'd have backcompat as @Ivanidzo4ka suggests, but since AFAIK this was never added anywhere really public except for debugging purposes, this might not be too essential? I think we can correct it later in the unlikely event it does prove essential.

yaeldMS added 2 commits February 15, 2019 13:57

Convert VectorToImageTransform to estimator/transformer

3b38183

Rename Schema, Row, etc.

091e9e7

yaeldekel requested review from Ivanidzo4ka and TomFinley February 15, 2019 22:17

Ivanidzo4ka mentioned this pull request Feb 18, 2019

Introduce order for pixel extraction #2602

Merged

TomFinley reviewed Feb 19, 2019

View reviewed changes

Code review comments, and fix baseline of EntryPointCatalog test

cc55f3c

Ivanidzo4ka reviewed Feb 19, 2019

View reviewed changes

yaeldMS added 2 commits February 19, 2019 12:24

Code review comments

c251746

Fix EntryPointCatalog baseline

d49bdc1

Ivanidzo4ka reviewed Feb 20, 2019

View reviewed changes

Ivanidzo4ka approved these changes Feb 20, 2019

View reviewed changes

zeahmed reviewed Feb 20, 2019

View reviewed changes

zeahmed approved these changes Feb 20, 2019

View reviewed changes

TomFinley approved these changes Feb 21, 2019

View reviewed changes

yaeldekel merged commit ed7f706 into dotnet:master Feb 21, 2019

yaeldekel deleted the vectortoimage branch February 21, 2019 18:03

yaeldekel mentioned this pull request Feb 21, 2019

Fix the build #2682

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VectorToImageTransform conversion to estimator/transformer #2580

VectorToImageTransform conversion to estimator/transformer #2580

yaeldekel commented Feb 15, 2019

TomFinley Feb 19, 2019 •

edited by yaeldekel

Loading

TomFinley Feb 19, 2019 •

edited by yaeldekel

Loading

Ivanidzo4ka Feb 19, 2019 •

edited by yaeldekel

Loading

Ivanidzo4ka Feb 19, 2019 •

edited by yaeldekel

Loading

Ivanidzo4ka Feb 19, 2019 •

edited by yaeldekel

Loading

codecov bot commented Feb 19, 2019 •

edited

Loading

Ivanidzo4ka Feb 20, 2019

Ivanidzo4ka left a comment

zeahmed Feb 20, 2019

zeahmed left a comment

TomFinley left a comment •

edited

Loading

VectorToImageTransform conversion to estimator/transformer #2580

VectorToImageTransform conversion to estimator/transformer #2580

Conversation

yaeldekel commented Feb 15, 2019

TomFinley Feb 19, 2019 • edited by yaeldekel Loading

Choose a reason for hiding this comment

TomFinley Feb 19, 2019 • edited by yaeldekel Loading

Choose a reason for hiding this comment

Ivanidzo4ka Feb 19, 2019 • edited by yaeldekel Loading

Choose a reason for hiding this comment

Ivanidzo4ka Feb 19, 2019 • edited by yaeldekel Loading

Choose a reason for hiding this comment

Ivanidzo4ka Feb 19, 2019 • edited by yaeldekel Loading

Choose a reason for hiding this comment

codecov bot commented Feb 19, 2019 • edited Loading

Codecov Report

Ivanidzo4ka Feb 20, 2019

Choose a reason for hiding this comment

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

zeahmed Feb 20, 2019

Choose a reason for hiding this comment

zeahmed left a comment

Choose a reason for hiding this comment

TomFinley left a comment • edited Loading

Choose a reason for hiding this comment

TomFinley Feb 19, 2019 •

edited by yaeldekel

Loading

TomFinley Feb 19, 2019 •

edited by yaeldekel

Loading

Ivanidzo4ka Feb 19, 2019 •

edited by yaeldekel

Loading

Ivanidzo4ka Feb 19, 2019 •

edited by yaeldekel

Loading

Ivanidzo4ka Feb 19, 2019 •

edited by yaeldekel

Loading

codecov bot commented Feb 19, 2019 •

edited

Loading

TomFinley left a comment •

edited

Loading