Uh oh!
There was an error while loading. Please reload this page.
Image support - #528
Conversation
| { | ||
| Contracts.CheckParam(height > 0, nameof(height)); | ||
| Contracts.CheckParam(width > 0, nameof(width)); | ||
| Contracts.CheckParam((long)height * width <= int.MaxValue / 4, nameof(height), "height * width is too large"); |
There was a problem hiding this comment.
height [](start = 92, length = 6)
nameof for params in this message? #Closed
| public override string ToString() | ||
| { | ||
| if (Height == 0 && Width == 0) | ||
| return "Picture"; |
There was a problem hiding this comment.
Picture [](start = 24, length = 7)
It's a little weird that the image type identifies itself as picture, is it not? #Closed
| return false; | ||
| if (Width != tmp.Width) | ||
| return false; | ||
| return true; |
There was a problem hiding this comment.
Maybe restructure as return Width == tmp.Width. #Closed
| if (other == this) | ||
| return true; | ||
| var tmp = other as ImageType; | ||
| if (tmp == null) |
There was a problem hiding this comment.
Will if (other as ImageType tmp == null) work? #Closed
| { | ||
| } | ||
| public override bool Equals(ColumnType other) |
There was a problem hiding this comment.
Equals [](start = 29, length = 6)
There's something a bit strange about this. ColumnType implements IEquatable<ColumnType>, and the guidelines for that say it should also overload Equals(object) and GetHashCode. Though to be fair to you I think all ColumnTypes have this problem. #Closed
There was a problem hiding this comment.
| @@ -0,0 +1,3 @@ | |||
| banana.jpg banana | |||
| hotdog.jpg hotdog | |||
| tomato.jpg tomato No newline at end of file | |||
There was a problem hiding this comment.
I feel like this should have been "not hotdog." 😃 #Closed
| namespace Microsoft.ML.Runtime.Data | ||
| { | ||
| // REVIEW: Rewrite as LambdaTransform to simplify. | ||
| public sealed class ImagePixelExtractorTransform : OneToOneTransformBase |
There was a problem hiding this comment.
ImagePixelExtractorTransform [](start = 24, length = 28)
Are you planning to add their entry point classes next? #Resolved
| namespace Microsoft.ML.Runtime.Data | ||
| { | ||
| // REVIEW: Rewrite as LambdaTransform to simplify. | ||
| public sealed class ImageResizerTransform : OneToOneTransformBase |
There was a problem hiding this comment.
Let's get in the rhythm of adding one line of
| namespace Microsoft.ML.Runtime.Data | ||
| { | ||
| // REVIEW: Rewrite as LambdaTransform to simplify. | ||
| public sealed class ImagePixelExtractorTransform : OneToOneTransformBase |
There was a problem hiding this comment.
ImagePixelExtractorTransform [](start = 24, length = 28)
Let's get in the rhythm of adding one line of
| namespace Microsoft.ML.Runtime.Data | ||
| { | ||
| // REVIEW: Rewrite as LambdaTransform to simplify. | ||
| public sealed class ImageLoaderTransform : OneToOneTransformBase |
There was a problem hiding this comment.
ImageLoaderTransform [](start = 24, length = 20)
summary #Resolved
| /// <summary> | ||
| /// Public constructor corresponding to SignatureDataTransform. | ||
| /// </summary> |
There was a problem hiding this comment.
I know we use this allover, and it's plenty meaningful as a code comment, but it will appear in the docs site as well, where might confuse users. Maybe turn it into a code comment removing the triple ///, and
Ideally add a meaningful line for the actual documentation. #Resolved
Ivanidzo4ka
commented
Jul 16, 2018
@dotnet-bot test Windows_NT Release |
Ivanidzo4ka
commented
Jul 17, 2018
@dotnet-bot test Linux Debug |
| { | ||
| Contracts.CheckParam(height > 0, nameof(height)); | ||
| Contracts.CheckParam(width > 0, nameof(width)); | ||
| Contracts.CheckParam((long)height * width <= int.MaxValue / 4, nameof(height), $"{nameof(height)} * {nameof(width)} is too large"); |
There was a problem hiding this comment.
$"{nameof(height)} * {nameof(width)} is too large" [](start = 91, length = 50)
If you really want it as format strings I'd encourage you to make this follow the if (...) throw Contracts.Except pattern. Or you could continue to make it a string literal by making this nameof(height) + " * " + nameof(width) + " is too large" or something. #Closed
| public string ImageFolder; | ||
| } | ||
| internal const string Summary = "Load images from a file."; |
There was a problem hiding this comment.
a file [](start = 58, length = 6)
You forgot to pluralize this. #Closed
There was a problem hiding this comment.
There was a problem hiding this comment.
Correct would be "load images from files." FYI @Ivanidzo4ka I feel like this is the only thing standing in the way of this being merged.
In reply to: 205597457 [](ancestors = 205597457,203182343)
Grayscale return exception catching
different image height and image width
Zruty0
commented
Jul 25, 2018
@Ivanidzo4ka , you have surely added a lot of code here :) I would much rather ask @TomFinley to finish his review, rather than jump in and try to grok all this myself. @TomFinley , if for some reason this doesn't work for you, please let me know and I'll start the review |
| private const string RegistrationName = "ImageGrayscale"; | ||
| /// Public constructor corresponding to SignatureDataTransform. |
There was a problem hiding this comment.
/// [](start = 8, length = 4)
Did you mean this to be // and not /// , since it is not really terribly sensible for callers? #Closed
| return Infos[iinfo].TypeSrc; | ||
| } | ||
| public ColorMatrix GreyscaleColorMatrix = new ColorMatrix( |
There was a problem hiding this comment.
public ColorMatrix GreyscaleColorMatrix [](start = 8, length = 39)
This is a mutable, public instance level field that affects the functionality of the transform.
Probably want this to be private (or at least internal if somehow it is referenced somewhere else), static, and almost certainly readonly. #Closed
| //verWrittenCur: 0x00010001, // Initial | ||
| verWrittenCur: 0x00010002, // Swith from OpenCV to Bitmap | ||
| verReadableCur: 0x00010002, | ||
| verWeCanReadBack: 0x00010002, |
There was a problem hiding this comment.
0x00010002 [](start = 34, length = 10)
What would happen if you were to read a 0x00010001 model? Would it just be a little bit off? Or is it just fundamentally different?
I think possibly it might be better to load those old models, just use the new types under the hood. If we were to, say, hypothetically change from using DvText to ReadOnlySpan<char> or something like that, we almost certainly would not refuse to load all models just on the basis of the type change alone. #Closed
There was a problem hiding this comment.
In past Loader/Resizer and Pixel extractor was tangled bundle which works together.
From what I found in my investigation during comparing System.Bitmap with old version, Pixel extractor was buggy and throw exception in case of rectangular shape (people used square shape all the time, but maybe rectangle image make sense as well) and Resizer instead of resizing image actually just pick subset of image from top left corner and count it as "Resized" image. Which is fine if you pass real width and height to transform, but if you actually want to resize them, not so great.
That's why I'm bumping versions in all 3 of them.
In reply to: 205264997 [](ancestors = 205264997)
There was a problem hiding this comment.
Ah OK. So they were not just somewhat different, the old ones were fundamentally broken. That's nice. I guess that's a good enough reason to break backcompat. #Closed
| private static VersionInfo GetVersionInfo() | ||
| { | ||
| return new VersionInfo( | ||
| modelSignature: "IMGGREY ", |
There was a problem hiding this comment.
IMGGREY [](start = 33, length = 8)
You had a signature of IMGLOADT for that. Why is this not IMGGREYT?
Also: is it intentional that this be GREY? Elsewhere we are using "gray."
If models already exist with this signature I guess we have to keep it, not sure if that's the case though since you didn't bump the version. :) (I'm not suggesting you bump the version to be clear.) #Closed
There was a problem hiding this comment.
English are so confusing, grey and gray is apparent to have same meaning, but my brain apparently throw coin during typing and at some point I had code with both spelling all across code. Which I tried to unified, but this one slip from my attention.
This is a new transform which we never had before, and I add it "just in case", and "maybe it will prove useful". So i think it make sense to start from version 1.
In reply to: 205265843 [](ancestors = 205265843)
There was a problem hiding this comment.
Oh sure @Ivanidzo4ka grey and gray are synonymous, though elsewhere you use gray. Also my impression is that we generally favor American spellings (gray) of words vs. British ones (grey).
...Even though I was brought up to use "grey," and had to correct myself since later in my life people thought I was weird. So now I hypercorrect, and you reap the benefits. :D
In reply to: 205597316 [](ancestors = 205597316,205265843)
TomFinley
left a comment
There was a problem hiding this comment.
Thank you @Ivanidzo4ka !
Ivanidzo4ka
commented
Jul 31, 2018
@dotnet-bot test VSTS: public-CI |
This reverts commit bdb742d.
Add images support based on System.Drawing
address #489
need create issue about IDataView datatype extensibility.