Skip to content

Changed Image classification API to accept Image as VBuffer<byte> #4242

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Oct 3, 2019
Merged

Changed Image classification API to accept Image as VBuffer<byte> #4242

merged 14 commits into from
Oct 3, 2019

Conversation

harshithapv
Copy link
Contributor

@harshithapv harshithapv commented Sep 23, 2019 •
edited by codemzs
Loading

Changed Image classification API to accept Image as V Buffer. Also, added a few optimizations to re-use the buffer for performance gains.
So we can do the best job, please check:

fixes #4153

@dnfclas
Copy link

dnfclas commented Sep 23, 2019 •
edited
Loading

CLA assistant check
All CLA requirements met.

@codemzs
Copy link
Member

codemzs commented Sep 29, 2019

Please revert this file, i.e git checkout upstream master filepath

Refers to: docs/samples/Microsoft.ML.Samples/Program.cs:32 in ae2ac0d. [](commit_id = ae2ac0d, deletion_comment = False)

@codemzs
Copy link
Member

codemzs commented Oct 3, 2019 •
edited by harshithapv
Loading 

            verWeCanReadBack: 0x00010002,

increment these versions since you are changing model format and then when deserializing the model check if the model was written with an older version or new version and based on that try to deserialize indicator for image type. #Resolved

Refers to: src/Microsoft.ML.ImageAnalytics/ImageLoader.cs:183 in 20d6660. [](commit_id = 20d6660, deletion_comment = False)

codemzs
codemzs approved these changes Oct 3, 2019
View reviewed changes
Copy link
Member

@codemzs codemzs left a comment

Choose a reason for hiding this comment

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

:shipit:

@codemzs codemzs merged commit ee743f7 into dotnet:master Oct 3, 2019
@harshithapv harshithapv mentioned this pull request Oct 3, 2019
Merged
yaeldekel
yaeldekel reviewed Oct 7, 2019
View reviewed changes
src/Microsoft.ML.ImageAnalytics/ImageLoader.cs
@@ -3,10 +3,13 @@
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Concurrent;
Copy link

Choose a reason for hiding this comment

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

Concurrent [](start = 25, length = 10)

This and System.Runtime.InteropServices are not used, please remove them.
Also, System.Diagnostics.Contracts should not be used, on line 328 you have Contract.Assert instead of Contracts.Assert.

yaeldekel
yaeldekel reviewed Oct 7, 2019
View reviewed changes
src/Microsoft.ML.ImageAnalytics/ImageLoader.cs
@@ -112,6 +135,10 @@ private ImageLoadingTransformer(IHost host, ModelLoadContext ctx)
// int: id of image folder

ImageFolder = ctx.LoadStringOrNull();
if (ctx.Header.ModelVerWritten >= 0x00010003) // do a version check
UseImageType = ctx.Reader.ReadBoolean();
Copy link

Choose a reason for hiding this comment

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

ReadBoolean [](start = 42, length = 11)

Please use ReadBoolByte instead.

yaeldekel
yaeldekel reviewed Oct 7, 2019
View reviewed changes
src/Microsoft.ML.ImageAnalytics/ImageLoader.cs
verWeCanReadBack: 0x00010002,
verWrittenCur: 0x00010003, // Added support for output type as byte array
verReadableCur: 0x00010003,
verWeCanReadBack: 0x00010003,
Copy link

Choose a reason for hiding this comment

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

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

This should still be 0x00010002, since you are taking care of backwards compatibility in line 138.

yaeldekel
yaeldekel reviewed Oct 7, 2019
View reviewed changes
src/Microsoft.ML.ImageAnalytics/ImageLoader.cs
private static bool TryLoadDataIntoBuffer(string path, ref VBuffer<byte> imgData, byte[] readBuffer)
{
int count = -1;
int bytesread = -1;
Copy link

Choose a reason for hiding this comment

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

These two don't need to be defined in advance.

CESARDELATORRE
CESARDELATORRE reviewed Oct 7, 2019
View reviewed changes
...icrosoft.ML.Samples/Dynamic/ImageClassification/ResnetV2101TransferLearningTrainTestSplit.cs
@@ -125,6 +131,9 @@ public static void Example()
IEnumerable<ImageData> testImages = LoadImagesFromDirectory(
imagesForPredictions, false);

byte[] imgBytes = File.ReadAllBytes(testImages.First().ImagePath);
VBuffer<Byte> imgData = new VBuffer<byte>(imgBytes.Length, imgBytes);
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look like this in-memory image is being used for the Prediction when using the PredictionEngine, right?
In fact, looks like this object 'imgData' is not used later on?

We need to have and test Predictions by providing the in-memory image instead of file paths.
I cannot find such a code.

harshithapv added a commit to harshithapv/machinelearning that referenced this pull request Oct 15, 2019
@harshithapv
Addressed a few of Yael's comments on PR dotnet#4242. Code cleaning. …
1c8f571 
…Added functionality to use resnet_v2_50 model. Added learning rate scheduling for CIFAR-10 dataset.
@harshithapv harshithapv mentioned this pull request Oct 15, 2019
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Mar 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@CESARDELATORRE CESARDELATORRE CESARDELATORRE left review comments

@eerhardt eerhardt eerhardt left review comments

@yaeldekel yaeldekel yaeldekel left review comments

@codemzs codemzs codemzs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Image Classification DNN Transfer Learning] - Support training and scoring with in-memory images
6 participants
@harshithapv @dnfclas @codemzs @CESARDELATORRE @eerhardt @yaeldekel