-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Introduce order for pixel extraction #2602
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
Introduce order for pixel extraction #2602
Conversation
/// <param name="interleave">Wheather to interleave the pixels, meaning keep them in the `ARGB ARGB` order, or leave them separated in the planar form, where the colors are outputed one by one | ||
/// alpha, red, green, blue for all the pixels of the image. </param> | ||
/// <param name="colors">What colors to extract.</param> | ||
/// <param name="order">In which order extract channels from pixel.</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
colors #Closed
[Argument(ArgumentType.AtMostOnce, HelpText = "Order of colors.")] | ||
public ImagePixelExtractingEstimator.ColorsOrder Order = Defaults.Order; | ||
|
||
[Argument(ArgumentType.AtMostOnce, HelpText = "Whether to separate each channel or interleave in specified order", ShortName = "interleave")] | ||
public bool InterleaveArgb = Defaults.Interleave; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InterleaveArgb [](start = 24, length = 14)
change to interleave since it's no longer argb #Resolved
Codecov Report
@@ Coverage Diff @@
## master #2602 +/- ##
=========================================
Coverage ? 71.66%
=========================================
Files ? 804
Lines ? 141988
Branches ? 16127
=========================================
Hits ? 101751
Misses ? 35795
Partials ? 4442
|
/// <param name="interleave">Whether the pixels are interleaved, meaning whether they are in `ARGB ARGB` order, or separated in the planar form, where the colors are specified one by one | ||
/// <param name="colors"> Specifies which <see cref="ImagePixelExtractingEstimator.ColorBits"/> are in the input pixel vectors. The order of colors specified in <paramref name="order"/></param> | ||
/// <param name="order">In which order extract colors presented in array.</param> | ||
/// <param name="interleave">Whether the pixels are interleaved, meaning whether they are in <paramref name="order"/> order, or separated in the planar form, where the colors are specified one by one | ||
/// alpha, red, green, blue for all the pixels of the image. </param> | ||
/// <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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change it #Closed
/// <param name="interleave">Whether the pixels are interleaved, meaning whether they are in `ARGB ARGB` order, or separated in the planar form, where the colors are specified one by one | ||
/// <param name="colors"> Specifies which <see cref="ImagePixelExtractingEstimator.ColorBits"/> are in the input pixel vectors. The order of colors specified in <paramref name="order"/></param> | ||
/// <param name="order">In which order extract colors presented in array.</param> | ||
/// <param name="interleave">Whether the pixels are interleaved, meaning whether they are in <paramref name="order"/> order, or separated in the planar form, where the colors are specified one by one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pixels [](start = 49, length = 6)
colors #Closed
/// <param name="offset">Offset color pixel value by this amount.</param> | ||
/// <param name="asFloat">Output the array as float array. If false, output as byte array.</param> | ||
/// <param name="colors">What colors to extract.</param> | ||
/// <param name="order">In which order extract colors from pixel.</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which order extract colors from pixel. [](start = 32, length = 41)
Would suggest:
In which order to extract colors from pixel.
(Same for the other extension) #Resolved
/// <param name="colors">What colors to extract.</param> | ||
/// <param name="order">In which order extract colors from pixel.</param> | ||
/// <param name="interleave">Whether to interleave the pixels colors, meaning keep them in the <paramref name="order"/> order, or leave them in the plannar form: | ||
/// first output one color values for all pixels, then another color and so on.</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one color values for all pixels [](start = 25, length = 31)
Maybe: "all the values for one color for all pixels, then all the values for another color and so on." #Resolved
/// <param name="colors"> Specifies which <see cref="ImagePixelExtractingEstimator.ColorBits"/> are in the input pixel vectors. The order of colors specified in <paramref name="order"/></param> | ||
/// <param name="order">In which order extract colors presented in array.</param> | ||
/// <param name="interleave">Whether the pixels are interleaved, meaning whether they are in <paramref name="order"/> order, or separated in the planar form, where the colors are specified one by one | ||
/// for all the pixels of the image. </param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you make this description and the one in the extension above one consistent?
@@ -85,7 +88,7 @@ internal bool TryUnparse(StringBuilder sb) | |||
{ | |||
Contracts.AssertValue(sb); | |||
if (UseAlpha != null || UseRed != null || UseGreen != null || UseBlue != null || Convert != null || | |||
Offset != null || Scale != null || InterleaveArgb != null) | |||
Offset != null || Scale != null || Interleave != null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you not need to add Order != null here? #Resolved
/// <param name="outputColumnName">Name of the column resulting from the transformation of <paramref name="inputColumnName"/>. Null means <paramref name="inputColumnName"/> is replaced.</param> | ||
/// <param name="inputColumnName">Name of the input column.</param> | ||
/// <param name="colors">What colors to extract.</param> | ||
/// <param name="interleave">Whether to interleave the pixels, meaning keep them in the `RGB RGB` order, or leave them in the plannar form: of all red pixels, | ||
/// than all green, than all blue.</param> | ||
/// <param name="order">In which order extract colors presented in array.</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extract [](start = 47, length = 7)
extracted? #Closed
@@ -150,16 +152,26 @@ public static VectorToImageConvertingEstimator ConvertToImage(this TransformsCat | |||
/// <param name="width">The width of the output images.</param> | |||
/// <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="colors"> Specifies which <see cref="ImagePixelExtractingEstimator.ColorBits"/> are in the input pixel vectors. The order of colors is: Alpha, Red, Green Blue.</param> | |||
/// <param name="interleave">Whether the pixels are interleaved, meaning whether they are in `ARGB ARGB` order, or separated in the planar form, where the colors are specified one by one | |||
/// <param name="colors"> Specifies which <see cref="ImagePixelExtractingEstimator.ColorBits"/> are in the input pixel vectors. The order of colors specified in <paramref name="order"/></param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
< [](start = 192, length = 2)
period at the end. #Closed
if (g) { vf[idst++] = (pb.G - offset) * scale; } | ||
if (b) { vf[idst++] = (pb.B - offset) * scale; } | ||
|
||
if (a != -1) { vf[idst + a] = (pb.A - offset) * scale; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra line. #Resolved
I don't see any test for extracting pixels in different color order. Do you have that? #Closed |
Not a blocking comment, but it would be very useful for future contributions to this code to know what planes represents. If you could add a line of comment on that it would be great. Refers to: src/Microsoft.ML.ImageAnalytics/ImagePixelExtractor.cs:325 in cee009e. [](commit_id = cee009e, deletion_comment = False) |
if (!vb.IsEmpty) | ||
{ | ||
for (int x = 0; x < w; x++, idstBase++) | ||
for (int x = 0; x < w; x++, idst++) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you refactor this by having the if...else if...else
loop inside the for loop you can simplify the code a bit. It would look like:
for... { var pb = src.GetPixel(x,y); if... else if... else... }
#Resolved
public readonly float Scale; | ||
|
||
/// <summary> Whether to interleave the pixels, meaning keep them in the `ARGB ARGB` order, or leave them separated in the plannar form.</summary> | ||
/// <summary>Whether to interleave the pixels colors, meaning keep them in the <see cref="Order"/> order, or leave them in the plannar form.</summary> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether to interleave the pixels colors, meaning keep them in the order, or leave them in the plannar form. [](start = 25, length = 127)
Could you double check with the catalog to make sure we have something consistent for this line? #Resolved
float scale = Defaults.Scale, | ||
bool asFloat = Defaults.Convert) | ||
: base(Contracts.CheckRef(env, nameof(env)).Register(nameof(ImagePixelExtractingEstimator)), | ||
new ImagePixelExtractingTransformer(env, outputColumnName, inputColumnName, colors, order, interleave, offset, scale, asFloat)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[](start = 120, length = 2)
nit: two spaces. #Resolved
@@ -84,33 +99,37 @@ internal bool TryUnparse(StringBuilder sb) | |||
{ | |||
Contracts.AssertValue(sb); | |||
if (ContainsAlpha != null || ContainsRed != null || ContainsGreen != null || ContainsBlue != null || ImageWidth != null || | |||
ImageHeight != null || Offset != null || Scale != null || InterleaveArgb != null) | |||
ImageHeight != null || Offset != null || Scale != null || Interleave != null || Order != null || DefaultAlpha != null || | |||
DefaultBlue != null || DefaultGreen != null || DefaultRed != null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Order? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// <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> | ||
/// <param name="defaultAlpha">Default value for alpha color, would be overriden if <paramref name="colors"/> contains <see cref="ImagePixelExtractingEstimator.ColorBits.Alpha"/>.</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think your code now has the opposite logic.
Would prefer to be consistent with the above extension here too. If you could also reorder them so that the first one applied comes first that would be great. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you talking about scale offset or defaultAlpha?
In reply to: 259550340 [](ancestors = 259550340,259550143)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I think codeflow did not pick up the right location for this. I meant to comment on scale
and offset
.
- I think the arguments should be presented in the same order in which they are applied.
- I think the order and the comment should match the above extension for ImageLoader
- I think the logic that you have in your code here does not match the comment:
machinelearning/src/Microsoft.ML.ImageAnalytics/VectorToImageTransform.cs
Lines 415 to 419 in cee009e
pixel = Color.FromArgb( ex.Alpha ? (int)Math.Round(alpha * scale - offset) : 0, (int)Math.Round(red * scale - offset), (int)Math.Round(green * scale - offset), (int)Math.Round(blue * scale - offset));
It seems that you are first scaling and the applying the offset. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I can do this: P-pixel x -VectorValue
p= xscale1-offset1
x =pscale2-offset2
this way I need to solve equation in order to pick up scale1 and offset1 if I know scale2 and offset2.
or I can have
x=(p-offset1)scale1
p = xscale2 - offset2
and relationship between offset1 = -offset2 and scale1 = 1/scale2
So in one transform i apply scale first, in another i apply offset.
I can make them same order in their parameters, but since I'm specifying which applied first and which second, it looks weird then you put one which applied second to first in argument list.
In reply to: 259551787 [](ancestors = 259551787)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for explaining the logic it makes sense like this!
In reply to: 259553517 [](ancestors = 259553517,259551787)
var pixels = new ImagePixelExtractingTransformer(env, "ImagePixels", "ImageCropped", ImagePixelExtractingEstimator.ColorBits.All, true, 2f / 255, 127.5f).Transform(cropped); | ||
IDataView backToBitmaps = new VectorToImageConvertingTransformer(env, "ImageRestored", "ImagePixels", | ||
imageHeight, imageWidth, ImagePixelExtractingEstimator.ColorBits.All, true, 255f / 2, -1f).Transform(pixels); | ||
var pixels = new ImagePixelExtractingTransformer(env, "ImagePixels", "ImageCropped", ImagePixelExtractingEstimator.ColorBits.All, interleave: true, scale: 2f/19, offset: 30).Transform(cropped); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cale: 2f/19, offset: 30) [](start = 161, length = 24)
why are the scale and offset changing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was hard to test. all first pixels are white which is 255 in each channel. (255-127.5)*(2/255) = 1.
And I didn't like old vector to image logic of scaling.
which was (pixel -offset)*scale.
Since this two transforms are tangle up together i prefer scale in PixelExtraction would be 1/scale in VectorToImage, and offset = -offset.
This way they opposite.
In reply to: 259550473 [](ancestors = 259550473)
you mean planes from above? In reply to: 466587738 [](ancestors = 466587738) Refers to: src/Microsoft.ML.ImageAnalytics/ImagePixelExtractor.cs:325 in cee009e. [](commit_id = cee009e, deletion_comment = False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks for doing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Ivan! It's great to see the new feature and more consistent and improved API :)
Fixes #2492