Samples and unit test for image-related transform estimators - #3165

Merged
abgoswam merged 4 commits into
dotnet:masterfrom
abgoswam:abgoswam/image_samples
Apr 3, 2019
Merged

Samples and unit test for image-related transform estimators#3165
abgoswam merged 4 commits into
dotnet:masterfrom
abgoswam:abgoswam/image_samples

Conversation

@abgoswam

Copy link
Copy Markdown
Member

Towards #1209

The PR makes the following changes

  • Adds unit test and sample for the ConvertToImage transform estimator.

  • Fixes the info presented to the user for the 4 existing image samples {ConvertToGrayscale, LoadImages, ExtractPixels, ResizeImages}

@abgoswamabgoswam changed the title Image samplesSamples and unit test for image-related transform estimatorsApr 2, 2019
@abgoswam
abgoswam requested a review from shmoradimsApril 2, 2019 01:32
// ImagePath : tomato.bmp
// Name : tomato
// ImageObject : System.Drawing.Bitmap
// Grayscale : System.Drawing.Bitmap

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe transpose, to give the column/row idea more accurately. #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

fixed


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

// Preview 1 row of the transformedData.
var transformedDataPreview = transformedData.Preview(1);
foreach (var kvPair in transformedDataPreview.RowView[0].Values)
{

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

{ [](start = 12, length = 1)

omit :) #Resolved

{
public static class ConvertToImage
{
private const int inputSize = 3 * 224 * 224;

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • 224 * 224; [](start = 39, length = 13)

might want to add a comment about what does each number represent #Resolved

Features = Enumerable.Repeat(0, inputSize).Select(x => random.NextDouble() * 100).ToArray()
};
}
}

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i'd move this below the example. If the users want to scroll to it they can. #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

fixed


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

// Preview 1 row of the transformedData.
var transformedDataPreview = transformedData.Preview(1);
foreach (var kvPair in transformedDataPreview.RowView[0].Values)
{

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

{ [](start = 11, length = 2)

omit #Resolved


// The transformedData IDataView contains the loaded and resized raw values

// Preview 1 row of the transformedData.

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no need for space in between. #Resolved

// Pixels : Dense vector of size 30000

Console.WriteLine("--------------------------------------------------");

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no need for it. Users can add it #Resolved

Console.WriteLine("--------------------------------------------------");

// Using schema comprehension to display raw pixels for each row.
// Display extracted pixels in column 'Pixels'.

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

might be easier to understand if put as:

Convert the transformedData into an IEnumerable #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

modified the pretty printing method


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


// Preview of the content of the images.tsv file
//
// ImagePath Name ImageObject "Pixels"

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

// ImagePath Name ImageObject [](start = 11, length = 41)

maybe keep the headers/column names, even if they don't print out. Or print them separately. #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

fixed


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

var pipeline = mlContext.Transforms.LoadImages("ImageObject", imagesFolder, "ImagePath");

var transformedData = pipeline.Fit(data).Transform(data);
// The transformedData IDataView contains the loaded images now

@Ivanidzo4kaIvanidzo4kaApr 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

now [](start = 72, length = 3)

dot in the end of sentence :) #Resolved

.Append(mlContext.Transforms.ResizeImages("ImageObjectResized", inputColumnName: "ImageObject", imageWidth: 100, imageHeight: 100));

var transformedData = pipeline.Fit(data).Transform(data);
// The transformedData IDataView contains the resized images now

@Ivanidzo4kaIvanidzo4kaApr 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

dot here is well #Resolved

@Ivanidzo4kaIvanidzo4ka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:shipit:

@codecov

codecovBot commented Apr 2, 2019

Copy link
Copy Markdown

Codecov Report

Merging #3165 into master will increase coverage by 0.01%.
The diff coverage is 94.73%.

@@ Coverage Diff @@## master #3165 +/- ##
==========================================
+ Coverage 72.53% 72.55% +0.01% 
==========================================
Files 808 807 -1 Lines 144775 144793 +18 Branches 16209 16211 +2 ==========================================
+ Hits 105012 105054 +42 + Misses 35348 35323 -25 - Partials 4415 4416 +1
FlagCoverage Δ
#Debug72.55% <94.73%> (+0.01%)⬆️
#production68.14% <ø> (+0.02%)⬆️
#test88.83% <94.73%> (ø)⬆️
Impacted FilesCoverage Δ
...c/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs16.66% <ø> (+5.55%)⬆️
test/Microsoft.ML.Tests/ImagesTests.cs98.69% <94.73%> (-0.13%)⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.1% <0%> (-0.16%)⬇️
src/Microsoft.ML.Data/Transforms/Normalizer.cs86.03% <0%> (ø)⬆️
...icrosoft.ML.Functional.Tests/DataTransformation.cs100% <0%> (ø)⬆️
...s/Api/CookbookSamples/CookbookSamplesDynamicApi.cs93.49% <0%> (ø)⬆️
...s/Scenarios/Api/CookbookSamples/CookbookSamples.cs99.49% <0%> (ø)⬆️
test/Microsoft.ML.Functional.Tests/Training.cs100% <0%> (ø)⬆️
...est/Microsoft.ML.Tests/FeatureContributionTests.cs98.55% <0%> (ø)⬆️
...oft.ML.Experimental/TransformsCatalogExtensions.cs
... and 5 more

@codecov

codecovBot commented Apr 2, 2019

Copy link
Copy Markdown

Codecov Report

Merging #3165 into master will increase coverage by 0.01%.
The diff coverage is 94.73%.

@@ Coverage Diff @@## master #3165 +/- ##
==========================================
+ Coverage 72.53% 72.55% +0.01% 
==========================================
Files 808 807 -1 Lines 144775 144793 +18 Branches 16209 16211 +2 ==========================================
+ Hits 105012 105054 +42 + Misses 35348 35322 -26 - Partials 4415 4417 +2
FlagCoverage Δ
#Debug72.55% <94.73%> (+0.01%)⬆️
#production68.14% <ø> (+0.02%)⬆️
#test88.83% <94.73%> (ø)⬆️
Impacted FilesCoverage Δ
...c/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs16.66% <ø> (+5.55%)⬆️
test/Microsoft.ML.Tests/ImagesTests.cs98.69% <94.73%> (-0.13%)⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.1% <0%> (-0.16%)⬇️
src/Microsoft.ML.Data/Transforms/Normalizer.cs86.03% <0%> (ø)⬆️
...icrosoft.ML.Functional.Tests/DataTransformation.cs100% <0%> (ø)⬆️
...s/Api/CookbookSamples/CookbookSamplesDynamicApi.cs93.49% <0%> (ø)⬆️
...s/Scenarios/Api/CookbookSamples/CookbookSamples.cs99.49% <0%> (ø)⬆️
test/Microsoft.ML.Functional.Tests/Training.cs100% <0%> (ø)⬆️
...est/Microsoft.ML.Tests/FeatureContributionTests.cs98.55% <0%> (ø)⬆️
...oft.ML.Experimental/TransformsCatalogExtensions.cs
... and 5 more

foreach (var row in data.RowView)
{
foreach (var kvPair in row.Values)
{

@shmoradimsshmoradimsApr 2, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

{ [](start = 16, length = 1)

for 1-liner blocks, let's not use {} to keep the samples short. similar to foreach loop on line 58 #Resolved

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

actually these are 2-liners. may use Linq to shorten it.


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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

keeping 2 for loops to keep logic simple


In reply to: 271453121 [](ancestors = 271453121,271452481)

private static void PrintPreview(DataDebuggerPreview data)
{
foreach (var colInfo in data.ColumnView)
Console.Write("{0, -25}", colInfo.Column.Name);

@shmoradimsshmoradimsApr 2, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

"{0, -25}" [](start = 30, length = 10)

let's just define this as:
var format = "{0,-25}" (they usually don't use space after comma)

and use 'format' in the rest of the code

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

not getting you .. whats meant by using 'format' ? is the suggestion to use string.Format somehow ..


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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

used control+k+d to fix format (if thats what u meant)


In reply to: 271462084 [](ancestors = 271462084,271455734)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ivan meant to define the "{0,-25}" as a variable, rather than repeating it, and reuse the variable below.


In reply to: 271465497 [](ancestors = 271465497,271462084,271455734)

var random = new Random(seed);

for (int i = 0; i < count; i++)
{

@shmoradimsshmoradimsApr 2, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

{ [](start = 12, length = 1)

omit #Resolved


private class DataPoint
{
[VectorType(inputSize)]

@sfilipisfilipiApr 3, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[VectorType(inputSize)] [](start = 11, length = 24)

is this annotation needed? #Resolved

@abgoswamabgoswamApr 3, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

yeah, this is indeed needed.

when fitting a model, we do some checks e.g. schema validation / size checks / whether the estimator works with fixed size or variable size etc

the VectorToImageConvertingEstimator works on fixed-size vector . without the annotation, the assumtion is that its a variable size vector , and the following check fails :

if(col.Kind!=SchemaShape.Column.VectorKind.Vector||(col.ItemType!=NumberDataViewType.Single&&col.ItemType!=NumberDataViewType.Double&&col.ItemType!=NumberDataViewType.Byte))

Unhandled Exception: System.ArgumentOutOfRangeException: Schema mismatch for input column 'Features': expected known-size vector of type Single, Double or Byte, got VarVector


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

@sfilipisfilipi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:shipit:

@abgoswam
abgoswam merged commit ac53748 into dotnet:masterApr 3, 2019
abgoswam added a commit to abgoswam/machinelearning that referenced this pull request Apr 5, 2019
…3165)
* updating image samples
* fix review comments (print preview)
* fix comments (minor nits)
* fix review comments (whitespaces)
@ghostghost locked as resolved and limited conversation to collaborators Mar 23, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@abgoswam@Ivanidzo4ka@shmoradims@sfilipi
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Samples and unit test for image-related transform estimators - #3165

Merged
abgoswam merged 4 commits into
dotnet:masterfrom
abgoswam:abgoswam/image_samples
Apr 3, 2019
Merged

Samples and unit test for image-related transform estimators#3165
abgoswam merged 4 commits into
dotnet:masterfrom
abgoswam:abgoswam/image_samples

Conversation

@abgoswam

Copy link
Copy Markdown
Member

Towards #1209

The PR makes the following changes

  • Adds unit test and sample for the ConvertToImage transform estimator.

  • Fixes the info presented to the user for the 4 existing image samples {ConvertToGrayscale, LoadImages, ExtractPixels, ResizeImages}

@abgoswamabgoswam changed the title Image samplesSamples and unit test for image-related transform estimatorsApr 2, 2019
@abgoswam
abgoswam requested a review from shmoradimsApril 2, 2019 01:32
// ImagePath : tomato.bmp
// Name : tomato
// ImageObject : System.Drawing.Bitmap
// Grayscale : System.Drawing.Bitmap

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe transpose, to give the column/row idea more accurately. #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

fixed


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

// Preview 1 row of the transformedData.
var transformedDataPreview = transformedData.Preview(1);
foreach (var kvPair in transformedDataPreview.RowView[0].Values)
{

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

{ [](start = 12, length = 1)

omit :) #Resolved

{
public static class ConvertToImage
{
private const int inputSize = 3 * 224 * 224;

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • 224 * 224; [](start = 39, length = 13)

might want to add a comment about what does each number represent #Resolved

Features = Enumerable.Repeat(0, inputSize).Select(x => random.NextDouble() * 100).ToArray()
};
}
}

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i'd move this below the example. If the users want to scroll to it they can. #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

fixed


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

// Preview 1 row of the transformedData.
var transformedDataPreview = transformedData.Preview(1);
foreach (var kvPair in transformedDataPreview.RowView[0].Values)
{

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

{ [](start = 11, length = 2)

omit #Resolved


// The transformedData IDataView contains the loaded and resized raw values

// Preview 1 row of the transformedData.

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no need for space in between. #Resolved

// Pixels : Dense vector of size 30000

Console.WriteLine("--------------------------------------------------");

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no need for it. Users can add it #Resolved

Console.WriteLine("--------------------------------------------------");

// Using schema comprehension to display raw pixels for each row.
// Display extracted pixels in column 'Pixels'.

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

might be easier to understand if put as:

Convert the transformedData into an IEnumerable #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

modified the pretty printing method


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


// Preview of the content of the images.tsv file
//
// ImagePath Name ImageObject "Pixels"

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

// ImagePath Name ImageObject [](start = 11, length = 41)

maybe keep the headers/column names, even if they don't print out. Or print them separately. #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

fixed


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

var pipeline = mlContext.Transforms.LoadImages("ImageObject", imagesFolder, "ImagePath");

var transformedData = pipeline.Fit(data).Transform(data);
// The transformedData IDataView contains the loaded images now

@Ivanidzo4kaIvanidzo4kaApr 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

now [](start = 72, length = 3)

dot in the end of sentence :) #Resolved

.Append(mlContext.Transforms.ResizeImages("ImageObjectResized", inputColumnName: "ImageObject", imageWidth: 100, imageHeight: 100));

var transformedData = pipeline.Fit(data).Transform(data);
// The transformedData IDataView contains the resized images now

@Ivanidzo4kaIvanidzo4kaApr 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

dot here is well #Resolved

@Ivanidzo4kaIvanidzo4ka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:shipit:

@codecov

codecovBot commented Apr 2, 2019

Copy link
Copy Markdown

Codecov Report

Merging #3165 into master will increase coverage by 0.01%.
The diff coverage is 94.73%.

@@ Coverage Diff @@## master #3165 +/- ##
==========================================
+ Coverage 72.53% 72.55% +0.01% 
==========================================
Files 808 807 -1 Lines 144775 144793 +18 Branches 16209 16211 +2 ==========================================
+ Hits 105012 105054 +42 + Misses 35348 35323 -25 - Partials 4415 4416 +1
FlagCoverage Δ
#Debug72.55% <94.73%> (+0.01%)⬆️
#production68.14% <ø> (+0.02%)⬆️
#test88.83% <94.73%> (ø)⬆️
Impacted FilesCoverage Δ
...c/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs16.66% <ø> (+5.55%)⬆️
test/Microsoft.ML.Tests/ImagesTests.cs98.69% <94.73%> (-0.13%)⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.1% <0%> (-0.16%)⬇️
src/Microsoft.ML.Data/Transforms/Normalizer.cs86.03% <0%> (ø)⬆️
...icrosoft.ML.Functional.Tests/DataTransformation.cs100% <0%> (ø)⬆️
...s/Api/CookbookSamples/CookbookSamplesDynamicApi.cs93.49% <0%> (ø)⬆️
...s/Scenarios/Api/CookbookSamples/CookbookSamples.cs99.49% <0%> (ø)⬆️
test/Microsoft.ML.Functional.Tests/Training.cs100% <0%> (ø)⬆️
...est/Microsoft.ML.Tests/FeatureContributionTests.cs98.55% <0%> (ø)⬆️
...oft.ML.Experimental/TransformsCatalogExtensions.cs
... and 5 more

@codecov

codecovBot commented Apr 2, 2019

Copy link
Copy Markdown

Codecov Report

Merging #3165 into master will increase coverage by 0.01%.
The diff coverage is 94.73%.

@@ Coverage Diff @@## master #3165 +/- ##
==========================================
+ Coverage 72.53% 72.55% +0.01% 
==========================================
Files 808 807 -1 Lines 144775 144793 +18 Branches 16209 16211 +2 ==========================================
+ Hits 105012 105054 +42 + Misses 35348 35322 -26 - Partials 4415 4417 +2
FlagCoverage Δ
#Debug72.55% <94.73%> (+0.01%)⬆️
#production68.14% <ø> (+0.02%)⬆️
#test88.83% <94.73%> (ø)⬆️
Impacted FilesCoverage Δ
...c/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs16.66% <ø> (+5.55%)⬆️
test/Microsoft.ML.Tests/ImagesTests.cs98.69% <94.73%> (-0.13%)⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.1% <0%> (-0.16%)⬇️
src/Microsoft.ML.Data/Transforms/Normalizer.cs86.03% <0%> (ø)⬆️
...icrosoft.ML.Functional.Tests/DataTransformation.cs100% <0%> (ø)⬆️
...s/Api/CookbookSamples/CookbookSamplesDynamicApi.cs93.49% <0%> (ø)⬆️
...s/Scenarios/Api/CookbookSamples/CookbookSamples.cs99.49% <0%> (ø)⬆️
test/Microsoft.ML.Functional.Tests/Training.cs100% <0%> (ø)⬆️
...est/Microsoft.ML.Tests/FeatureContributionTests.cs98.55% <0%> (ø)⬆️
...oft.ML.Experimental/TransformsCatalogExtensions.cs
... and 5 more

foreach (var row in data.RowView)
{
foreach (var kvPair in row.Values)
{

@shmoradimsshmoradimsApr 2, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

{ [](start = 16, length = 1)

for 1-liner blocks, let's not use {} to keep the samples short. similar to foreach loop on line 58 #Resolved

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

actually these are 2-liners. may use Linq to shorten it.


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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

keeping 2 for loops to keep logic simple


In reply to: 271453121 [](ancestors = 271453121,271452481)

private static void PrintPreview(DataDebuggerPreview data)
{
foreach (var colInfo in data.ColumnView)
Console.Write("{0, -25}", colInfo.Column.Name);

@shmoradimsshmoradimsApr 2, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

"{0, -25}" [](start = 30, length = 10)

let's just define this as:
var format = "{0,-25}" (they usually don't use space after comma)

and use 'format' in the rest of the code

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

not getting you .. whats meant by using 'format' ? is the suggestion to use string.Format somehow ..


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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

used control+k+d to fix format (if thats what u meant)


In reply to: 271462084 [](ancestors = 271462084,271455734)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ivan meant to define the "{0,-25}" as a variable, rather than repeating it, and reuse the variable below.


In reply to: 271465497 [](ancestors = 271465497,271462084,271455734)

var random = new Random(seed);

for (int i = 0; i < count; i++)
{

@shmoradimsshmoradimsApr 2, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

{ [](start = 12, length = 1)

omit #Resolved


private class DataPoint
{
[VectorType(inputSize)]

@sfilipisfilipiApr 3, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[VectorType(inputSize)] [](start = 11, length = 24)

is this annotation needed? #Resolved

@abgoswamabgoswamApr 3, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

yeah, this is indeed needed.

when fitting a model, we do some checks e.g. schema validation / size checks / whether the estimator works with fixed size or variable size etc

the VectorToImageConvertingEstimator works on fixed-size vector . without the annotation, the assumtion is that its a variable size vector , and the following check fails :

if(col.Kind!=SchemaShape.Column.VectorKind.Vector||(col.ItemType!=NumberDataViewType.Single&&col.ItemType!=NumberDataViewType.Double&&col.ItemType!=NumberDataViewType.Byte))

Unhandled Exception: System.ArgumentOutOfRangeException: Schema mismatch for input column 'Features': expected known-size vector of type Single, Double or Byte, got VarVector


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

@sfilipisfilipi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:shipit:

@abgoswam
abgoswam merged commit ac53748 into dotnet:masterApr 3, 2019
abgoswam added a commit to abgoswam/machinelearning that referenced this pull request Apr 5, 2019
…3165)
* updating image samples
* fix review comments (print preview)
* fix comments (minor nits)
* fix review comments (whitespaces)
@ghostghost locked as resolved and limited conversation to collaborators Mar 23, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@abgoswam@Ivanidzo4ka@shmoradims@sfilipi
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Samples and unit test for image-related transform estimators - #3165

Merged
abgoswam merged 4 commits into
dotnet:masterfrom
abgoswam:abgoswam/image_samples
Apr 3, 2019
Merged

Samples and unit test for image-related transform estimators#3165
abgoswam merged 4 commits into
dotnet:masterfrom
abgoswam:abgoswam/image_samples

Conversation

@abgoswam

Copy link
Copy Markdown
Member

Towards #1209

The PR makes the following changes

  • Adds unit test and sample for the ConvertToImage transform estimator.

  • Fixes the info presented to the user for the 4 existing image samples {ConvertToGrayscale, LoadImages, ExtractPixels, ResizeImages}

@abgoswamabgoswam changed the title Image samplesSamples and unit test for image-related transform estimatorsApr 2, 2019
@abgoswam
abgoswam requested a review from shmoradimsApril 2, 2019 01:32
// ImagePath : tomato.bmp
// Name : tomato
// ImageObject : System.Drawing.Bitmap
// Grayscale : System.Drawing.Bitmap

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe transpose, to give the column/row idea more accurately. #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

fixed


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

// Preview 1 row of the transformedData.
var transformedDataPreview = transformedData.Preview(1);
foreach (var kvPair in transformedDataPreview.RowView[0].Values)
{

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

{ [](start = 12, length = 1)

omit :) #Resolved

{
public static class ConvertToImage
{
private const int inputSize = 3 * 224 * 224;

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • 224 * 224; [](start = 39, length = 13)

might want to add a comment about what does each number represent #Resolved

Features = Enumerable.Repeat(0, inputSize).Select(x => random.NextDouble() * 100).ToArray()
};
}
}

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i'd move this below the example. If the users want to scroll to it they can. #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

fixed


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

// Preview 1 row of the transformedData.
var transformedDataPreview = transformedData.Preview(1);
foreach (var kvPair in transformedDataPreview.RowView[0].Values)
{

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

{ [](start = 11, length = 2)

omit #Resolved


// The transformedData IDataView contains the loaded and resized raw values

// Preview 1 row of the transformedData.

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no need for space in between. #Resolved

// Pixels : Dense vector of size 30000

Console.WriteLine("--------------------------------------------------");

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no need for it. Users can add it #Resolved

Console.WriteLine("--------------------------------------------------");

// Using schema comprehension to display raw pixels for each row.
// Display extracted pixels in column 'Pixels'.

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

might be easier to understand if put as:

Convert the transformedData into an IEnumerable #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

modified the pretty printing method


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


// Preview of the content of the images.tsv file
//
// ImagePath Name ImageObject "Pixels"

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

// ImagePath Name ImageObject [](start = 11, length = 41)

maybe keep the headers/column names, even if they don't print out. Or print them separately. #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

fixed


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

var pipeline = mlContext.Transforms.LoadImages("ImageObject", imagesFolder, "ImagePath");

var transformedData = pipeline.Fit(data).Transform(data);
// The transformedData IDataView contains the loaded images now

@Ivanidzo4kaIvanidzo4kaApr 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

now [](start = 72, length = 3)

dot in the end of sentence :) #Resolved

.Append(mlContext.Transforms.ResizeImages("ImageObjectResized", inputColumnName: "ImageObject", imageWidth: 100, imageHeight: 100));

var transformedData = pipeline.Fit(data).Transform(data);
// The transformedData IDataView contains the resized images now

@Ivanidzo4kaIvanidzo4kaApr 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

dot here is well #Resolved

@Ivanidzo4kaIvanidzo4ka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:shipit:

@codecov

codecovBot commented Apr 2, 2019

Copy link
Copy Markdown

Codecov Report

Merging #3165 into master will increase coverage by 0.01%.
The diff coverage is 94.73%.

@@ Coverage Diff @@## master #3165 +/- ##
==========================================
+ Coverage 72.53% 72.55% +0.01% 
==========================================
Files 808 807 -1 Lines 144775 144793 +18 Branches 16209 16211 +2 ==========================================
+ Hits 105012 105054 +42 + Misses 35348 35323 -25 - Partials 4415 4416 +1
FlagCoverage Δ
#Debug72.55% <94.73%> (+0.01%)⬆️
#production68.14% <ø> (+0.02%)⬆️
#test88.83% <94.73%> (ø)⬆️
Impacted FilesCoverage Δ
...c/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs16.66% <ø> (+5.55%)⬆️
test/Microsoft.ML.Tests/ImagesTests.cs98.69% <94.73%> (-0.13%)⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.1% <0%> (-0.16%)⬇️
src/Microsoft.ML.Data/Transforms/Normalizer.cs86.03% <0%> (ø)⬆️
...icrosoft.ML.Functional.Tests/DataTransformation.cs100% <0%> (ø)⬆️
...s/Api/CookbookSamples/CookbookSamplesDynamicApi.cs93.49% <0%> (ø)⬆️
...s/Scenarios/Api/CookbookSamples/CookbookSamples.cs99.49% <0%> (ø)⬆️
test/Microsoft.ML.Functional.Tests/Training.cs100% <0%> (ø)⬆️
...est/Microsoft.ML.Tests/FeatureContributionTests.cs98.55% <0%> (ø)⬆️
...oft.ML.Experimental/TransformsCatalogExtensions.cs
... and 5 more

@codecov

codecovBot commented Apr 2, 2019

Copy link
Copy Markdown

Codecov Report

Merging #3165 into master will increase coverage by 0.01%.
The diff coverage is 94.73%.

@@ Coverage Diff @@## master #3165 +/- ##
==========================================
+ Coverage 72.53% 72.55% +0.01% 
==========================================
Files 808 807 -1 Lines 144775 144793 +18 Branches 16209 16211 +2 ==========================================
+ Hits 105012 105054 +42 + Misses 35348 35322 -26 - Partials 4415 4417 +2
FlagCoverage Δ
#Debug72.55% <94.73%> (+0.01%)⬆️
#production68.14% <ø> (+0.02%)⬆️
#test88.83% <94.73%> (ø)⬆️
Impacted FilesCoverage Δ
...c/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs16.66% <ø> (+5.55%)⬆️
test/Microsoft.ML.Tests/ImagesTests.cs98.69% <94.73%> (-0.13%)⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.1% <0%> (-0.16%)⬇️
src/Microsoft.ML.Data/Transforms/Normalizer.cs86.03% <0%> (ø)⬆️
...icrosoft.ML.Functional.Tests/DataTransformation.cs100% <0%> (ø)⬆️
...s/Api/CookbookSamples/CookbookSamplesDynamicApi.cs93.49% <0%> (ø)⬆️
...s/Scenarios/Api/CookbookSamples/CookbookSamples.cs99.49% <0%> (ø)⬆️
test/Microsoft.ML.Functional.Tests/Training.cs100% <0%> (ø)⬆️
...est/Microsoft.ML.Tests/FeatureContributionTests.cs98.55% <0%> (ø)⬆️
...oft.ML.Experimental/TransformsCatalogExtensions.cs
... and 5 more

foreach (var row in data.RowView)
{
foreach (var kvPair in row.Values)
{

@shmoradimsshmoradimsApr 2, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

{ [](start = 16, length = 1)

for 1-liner blocks, let's not use {} to keep the samples short. similar to foreach loop on line 58 #Resolved

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

actually these are 2-liners. may use Linq to shorten it.


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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

keeping 2 for loops to keep logic simple


In reply to: 271453121 [](ancestors = 271453121,271452481)

private static void PrintPreview(DataDebuggerPreview data)
{
foreach (var colInfo in data.ColumnView)
Console.Write("{0, -25}", colInfo.Column.Name);

@shmoradimsshmoradimsApr 2, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

"{0, -25}" [](start = 30, length = 10)

let's just define this as:
var format = "{0,-25}" (they usually don't use space after comma)

and use 'format' in the rest of the code

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

not getting you .. whats meant by using 'format' ? is the suggestion to use string.Format somehow ..


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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

used control+k+d to fix format (if thats what u meant)


In reply to: 271462084 [](ancestors = 271462084,271455734)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ivan meant to define the "{0,-25}" as a variable, rather than repeating it, and reuse the variable below.


In reply to: 271465497 [](ancestors = 271465497,271462084,271455734)

var random = new Random(seed);

for (int i = 0; i < count; i++)
{

@shmoradimsshmoradimsApr 2, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

{ [](start = 12, length = 1)

omit #Resolved


private class DataPoint
{
[VectorType(inputSize)]

@sfilipisfilipiApr 3, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[VectorType(inputSize)] [](start = 11, length = 24)

is this annotation needed? #Resolved

@abgoswamabgoswamApr 3, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

yeah, this is indeed needed.

when fitting a model, we do some checks e.g. schema validation / size checks / whether the estimator works with fixed size or variable size etc

the VectorToImageConvertingEstimator works on fixed-size vector . without the annotation, the assumtion is that its a variable size vector , and the following check fails :

if(col.Kind!=SchemaShape.Column.VectorKind.Vector||(col.ItemType!=NumberDataViewType.Single&&col.ItemType!=NumberDataViewType.Double&&col.ItemType!=NumberDataViewType.Byte))

Unhandled Exception: System.ArgumentOutOfRangeException: Schema mismatch for input column 'Features': expected known-size vector of type Single, Double or Byte, got VarVector


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

@sfilipisfilipi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:shipit:

@abgoswam
abgoswam merged commit ac53748 into dotnet:masterApr 3, 2019
abgoswam added a commit to abgoswam/machinelearning that referenced this pull request Apr 5, 2019
…3165)
* updating image samples
* fix review comments (print preview)
* fix comments (minor nits)
* fix review comments (whitespaces)
@ghostghost locked as resolved and limited conversation to collaborators Mar 23, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@abgoswam@Ivanidzo4ka@shmoradims@sfilipi
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Samples and unit test for image-related transform estimators - #3165

Merged
abgoswam merged 4 commits into
dotnet:masterfrom
abgoswam:abgoswam/image_samples
Apr 3, 2019
Merged

Samples and unit test for image-related transform estimators#3165
abgoswam merged 4 commits into
dotnet:masterfrom
abgoswam:abgoswam/image_samples

Conversation

@abgoswam

Copy link
Copy Markdown
Member

Towards #1209

The PR makes the following changes

  • Adds unit test and sample for the ConvertToImage transform estimator.

  • Fixes the info presented to the user for the 4 existing image samples {ConvertToGrayscale, LoadImages, ExtractPixels, ResizeImages}

@abgoswamabgoswam changed the title Image samplesSamples and unit test for image-related transform estimatorsApr 2, 2019
@abgoswam
abgoswam requested a review from shmoradimsApril 2, 2019 01:32
// ImagePath : tomato.bmp
// Name : tomato
// ImageObject : System.Drawing.Bitmap
// Grayscale : System.Drawing.Bitmap

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe transpose, to give the column/row idea more accurately. #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

fixed


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

// Preview 1 row of the transformedData.
var transformedDataPreview = transformedData.Preview(1);
foreach (var kvPair in transformedDataPreview.RowView[0].Values)
{

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

{ [](start = 12, length = 1)

omit :) #Resolved

{
public static class ConvertToImage
{
private const int inputSize = 3 * 224 * 224;

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • 224 * 224; [](start = 39, length = 13)

might want to add a comment about what does each number represent #Resolved

Features = Enumerable.Repeat(0, inputSize).Select(x => random.NextDouble() * 100).ToArray()
};
}
}

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i'd move this below the example. If the users want to scroll to it they can. #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

fixed


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

// Preview 1 row of the transformedData.
var transformedDataPreview = transformedData.Preview(1);
foreach (var kvPair in transformedDataPreview.RowView[0].Values)
{

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

{ [](start = 11, length = 2)

omit #Resolved


// The transformedData IDataView contains the loaded and resized raw values

// Preview 1 row of the transformedData.

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no need for space in between. #Resolved

// Pixels : Dense vector of size 30000

Console.WriteLine("--------------------------------------------------");

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no need for it. Users can add it #Resolved

Console.WriteLine("--------------------------------------------------");

// Using schema comprehension to display raw pixels for each row.
// Display extracted pixels in column 'Pixels'.

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

might be easier to understand if put as:

Convert the transformedData into an IEnumerable #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

modified the pretty printing method


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


// Preview of the content of the images.tsv file
//
// ImagePath Name ImageObject "Pixels"

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

// ImagePath Name ImageObject [](start = 11, length = 41)

maybe keep the headers/column names, even if they don't print out. Or print them separately. #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

fixed


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

var pipeline = mlContext.Transforms.LoadImages("ImageObject", imagesFolder, "ImagePath");

var transformedData = pipeline.Fit(data).Transform(data);
// The transformedData IDataView contains the loaded images now

@Ivanidzo4kaIvanidzo4kaApr 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

now [](start = 72, length = 3)

dot in the end of sentence :) #Resolved

.Append(mlContext.Transforms.ResizeImages("ImageObjectResized", inputColumnName: "ImageObject", imageWidth: 100, imageHeight: 100));

var transformedData = pipeline.Fit(data).Transform(data);
// The transformedData IDataView contains the resized images now

@Ivanidzo4kaIvanidzo4kaApr 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

dot here is well #Resolved

@Ivanidzo4kaIvanidzo4ka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:shipit:

@codecov

codecovBot commented Apr 2, 2019

Copy link
Copy Markdown

Codecov Report

Merging #3165 into master will increase coverage by 0.01%.
The diff coverage is 94.73%.

@@ Coverage Diff @@## master #3165 +/- ##
==========================================
+ Coverage 72.53% 72.55% +0.01% 
==========================================
Files 808 807 -1 Lines 144775 144793 +18 Branches 16209 16211 +2 ==========================================
+ Hits 105012 105054 +42 + Misses 35348 35323 -25 - Partials 4415 4416 +1
FlagCoverage Δ
#Debug72.55% <94.73%> (+0.01%)⬆️
#production68.14% <ø> (+0.02%)⬆️
#test88.83% <94.73%> (ø)⬆️
Impacted FilesCoverage Δ
...c/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs16.66% <ø> (+5.55%)⬆️
test/Microsoft.ML.Tests/ImagesTests.cs98.69% <94.73%> (-0.13%)⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.1% <0%> (-0.16%)⬇️
src/Microsoft.ML.Data/Transforms/Normalizer.cs86.03% <0%> (ø)⬆️
...icrosoft.ML.Functional.Tests/DataTransformation.cs100% <0%> (ø)⬆️
...s/Api/CookbookSamples/CookbookSamplesDynamicApi.cs93.49% <0%> (ø)⬆️
...s/Scenarios/Api/CookbookSamples/CookbookSamples.cs99.49% <0%> (ø)⬆️
test/Microsoft.ML.Functional.Tests/Training.cs100% <0%> (ø)⬆️
...est/Microsoft.ML.Tests/FeatureContributionTests.cs98.55% <0%> (ø)⬆️
...oft.ML.Experimental/TransformsCatalogExtensions.cs
... and 5 more

@codecov

codecovBot commented Apr 2, 2019

Copy link
Copy Markdown

Codecov Report

Merging #3165 into master will increase coverage by 0.01%.
The diff coverage is 94.73%.

@@ Coverage Diff @@## master #3165 +/- ##
==========================================
+ Coverage 72.53% 72.55% +0.01% 
==========================================
Files 808 807 -1 Lines 144775 144793 +18 Branches 16209 16211 +2 ==========================================
+ Hits 105012 105054 +42 + Misses 35348 35322 -26 - Partials 4415 4417 +2
FlagCoverage Δ
#Debug72.55% <94.73%> (+0.01%)⬆️
#production68.14% <ø> (+0.02%)⬆️
#test88.83% <94.73%> (ø)⬆️
Impacted FilesCoverage Δ
...c/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs16.66% <ø> (+5.55%)⬆️
test/Microsoft.ML.Tests/ImagesTests.cs98.69% <94.73%> (-0.13%)⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.1% <0%> (-0.16%)⬇️
src/Microsoft.ML.Data/Transforms/Normalizer.cs86.03% <0%> (ø)⬆️
...icrosoft.ML.Functional.Tests/DataTransformation.cs100% <0%> (ø)⬆️
...s/Api/CookbookSamples/CookbookSamplesDynamicApi.cs93.49% <0%> (ø)⬆️
...s/Scenarios/Api/CookbookSamples/CookbookSamples.cs99.49% <0%> (ø)⬆️
test/Microsoft.ML.Functional.Tests/Training.cs100% <0%> (ø)⬆️
...est/Microsoft.ML.Tests/FeatureContributionTests.cs98.55% <0%> (ø)⬆️
...oft.ML.Experimental/TransformsCatalogExtensions.cs
... and 5 more

foreach (var row in data.RowView)
{
foreach (var kvPair in row.Values)
{

@shmoradimsshmoradimsApr 2, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

{ [](start = 16, length = 1)

for 1-liner blocks, let's not use {} to keep the samples short. similar to foreach loop on line 58 #Resolved

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

actually these are 2-liners. may use Linq to shorten it.


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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

keeping 2 for loops to keep logic simple


In reply to: 271453121 [](ancestors = 271453121,271452481)

private static void PrintPreview(DataDebuggerPreview data)
{
foreach (var colInfo in data.ColumnView)
Console.Write("{0, -25}", colInfo.Column.Name);

@shmoradimsshmoradimsApr 2, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

"{0, -25}" [](start = 30, length = 10)

let's just define this as:
var format = "{0,-25}" (they usually don't use space after comma)

and use 'format' in the rest of the code

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

not getting you .. whats meant by using 'format' ? is the suggestion to use string.Format somehow ..


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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

used control+k+d to fix format (if thats what u meant)


In reply to: 271462084 [](ancestors = 271462084,271455734)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ivan meant to define the "{0,-25}" as a variable, rather than repeating it, and reuse the variable below.


In reply to: 271465497 [](ancestors = 271465497,271462084,271455734)

var random = new Random(seed);

for (int i = 0; i < count; i++)
{

@shmoradimsshmoradimsApr 2, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

{ [](start = 12, length = 1)

omit #Resolved


private class DataPoint
{
[VectorType(inputSize)]

@sfilipisfilipiApr 3, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[VectorType(inputSize)] [](start = 11, length = 24)

is this annotation needed? #Resolved

@abgoswamabgoswamApr 3, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

yeah, this is indeed needed.

when fitting a model, we do some checks e.g. schema validation / size checks / whether the estimator works with fixed size or variable size etc

the VectorToImageConvertingEstimator works on fixed-size vector . without the annotation, the assumtion is that its a variable size vector , and the following check fails :

if(col.Kind!=SchemaShape.Column.VectorKind.Vector||(col.ItemType!=NumberDataViewType.Single&&col.ItemType!=NumberDataViewType.Double&&col.ItemType!=NumberDataViewType.Byte))

Unhandled Exception: System.ArgumentOutOfRangeException: Schema mismatch for input column 'Features': expected known-size vector of type Single, Double or Byte, got VarVector


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

@sfilipisfilipi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:shipit:

@abgoswam
abgoswam merged commit ac53748 into dotnet:masterApr 3, 2019
abgoswam added a commit to abgoswam/machinelearning that referenced this pull request Apr 5, 2019
…3165)
* updating image samples
* fix review comments (print preview)
* fix comments (minor nits)
* fix review comments (whitespaces)
@ghostghost locked as resolved and limited conversation to collaborators Mar 23, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@abgoswam@Ivanidzo4ka@shmoradims@sfilipi
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Samples and unit test for image-related transform estimators - #3165

Merged
abgoswam merged 4 commits into
dotnet:masterfrom
abgoswam:abgoswam/image_samples
Apr 3, 2019
Merged

Samples and unit test for image-related transform estimators#3165
abgoswam merged 4 commits into
dotnet:masterfrom
abgoswam:abgoswam/image_samples

Conversation

@abgoswam

Copy link
Copy Markdown
Member

Towards #1209

The PR makes the following changes

  • Adds unit test and sample for the ConvertToImage transform estimator.

  • Fixes the info presented to the user for the 4 existing image samples {ConvertToGrayscale, LoadImages, ExtractPixels, ResizeImages}

@abgoswamabgoswam changed the title Image samplesSamples and unit test for image-related transform estimatorsApr 2, 2019
@abgoswam
abgoswam requested a review from shmoradimsApril 2, 2019 01:32
// ImagePath : tomato.bmp
// Name : tomato
// ImageObject : System.Drawing.Bitmap
// Grayscale : System.Drawing.Bitmap

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe transpose, to give the column/row idea more accurately. #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

fixed


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

// Preview 1 row of the transformedData.
var transformedDataPreview = transformedData.Preview(1);
foreach (var kvPair in transformedDataPreview.RowView[0].Values)
{

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

{ [](start = 12, length = 1)

omit :) #Resolved

{
public static class ConvertToImage
{
private const int inputSize = 3 * 224 * 224;

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • 224 * 224; [](start = 39, length = 13)

might want to add a comment about what does each number represent #Resolved

Features = Enumerable.Repeat(0, inputSize).Select(x => random.NextDouble() * 100).ToArray()
};
}
}

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i'd move this below the example. If the users want to scroll to it they can. #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

fixed


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

// Preview 1 row of the transformedData.
var transformedDataPreview = transformedData.Preview(1);
foreach (var kvPair in transformedDataPreview.RowView[0].Values)
{

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

{ [](start = 11, length = 2)

omit #Resolved


// The transformedData IDataView contains the loaded and resized raw values

// Preview 1 row of the transformedData.

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no need for space in between. #Resolved

// Pixels : Dense vector of size 30000

Console.WriteLine("--------------------------------------------------");

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no need for it. Users can add it #Resolved

Console.WriteLine("--------------------------------------------------");

// Using schema comprehension to display raw pixels for each row.
// Display extracted pixels in column 'Pixels'.

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

might be easier to understand if put as:

Convert the transformedData into an IEnumerable #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

modified the pretty printing method


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


// Preview of the content of the images.tsv file
//
// ImagePath Name ImageObject "Pixels"

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

// ImagePath Name ImageObject [](start = 11, length = 41)

maybe keep the headers/column names, even if they don't print out. Or print them separately. #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

fixed


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

var pipeline = mlContext.Transforms.LoadImages("ImageObject", imagesFolder, "ImagePath");

var transformedData = pipeline.Fit(data).Transform(data);
// The transformedData IDataView contains the loaded images now

@Ivanidzo4kaIvanidzo4kaApr 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

now [](start = 72, length = 3)

dot in the end of sentence :) #Resolved

.Append(mlContext.Transforms.ResizeImages("ImageObjectResized", inputColumnName: "ImageObject", imageWidth: 100, imageHeight: 100));

var transformedData = pipeline.Fit(data).Transform(data);
// The transformedData IDataView contains the resized images now

@Ivanidzo4kaIvanidzo4kaApr 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

dot here is well #Resolved

@Ivanidzo4kaIvanidzo4ka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:shipit:

@codecov

codecovBot commented Apr 2, 2019

Copy link
Copy Markdown

Codecov Report

Merging #3165 into master will increase coverage by 0.01%.
The diff coverage is 94.73%.

@@ Coverage Diff @@## master #3165 +/- ##
==========================================
+ Coverage 72.53% 72.55% +0.01% 
==========================================
Files 808 807 -1 Lines 144775 144793 +18 Branches 16209 16211 +2 ==========================================
+ Hits 105012 105054 +42 + Misses 35348 35323 -25 - Partials 4415 4416 +1
FlagCoverage Δ
#Debug72.55% <94.73%> (+0.01%)⬆️
#production68.14% <ø> (+0.02%)⬆️
#test88.83% <94.73%> (ø)⬆️
Impacted FilesCoverage Δ
...c/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs16.66% <ø> (+5.55%)⬆️
test/Microsoft.ML.Tests/ImagesTests.cs98.69% <94.73%> (-0.13%)⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.1% <0%> (-0.16%)⬇️
src/Microsoft.ML.Data/Transforms/Normalizer.cs86.03% <0%> (ø)⬆️
...icrosoft.ML.Functional.Tests/DataTransformation.cs100% <0%> (ø)⬆️
...s/Api/CookbookSamples/CookbookSamplesDynamicApi.cs93.49% <0%> (ø)⬆️
...s/Scenarios/Api/CookbookSamples/CookbookSamples.cs99.49% <0%> (ø)⬆️
test/Microsoft.ML.Functional.Tests/Training.cs100% <0%> (ø)⬆️
...est/Microsoft.ML.Tests/FeatureContributionTests.cs98.55% <0%> (ø)⬆️
...oft.ML.Experimental/TransformsCatalogExtensions.cs
... and 5 more

@codecov

codecovBot commented Apr 2, 2019

Copy link
Copy Markdown

Codecov Report

Merging #3165 into master will increase coverage by 0.01%.
The diff coverage is 94.73%.

@@ Coverage Diff @@## master #3165 +/- ##
==========================================
+ Coverage 72.53% 72.55% +0.01% 
==========================================
Files 808 807 -1 Lines 144775 144793 +18 Branches 16209 16211 +2 ==========================================
+ Hits 105012 105054 +42 + Misses 35348 35322 -26 - Partials 4415 4417 +2
FlagCoverage Δ
#Debug72.55% <94.73%> (+0.01%)⬆️
#production68.14% <ø> (+0.02%)⬆️
#test88.83% <94.73%> (ø)⬆️
Impacted FilesCoverage Δ
...c/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs16.66% <ø> (+5.55%)⬆️
test/Microsoft.ML.Tests/ImagesTests.cs98.69% <94.73%> (-0.13%)⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.1% <0%> (-0.16%)⬇️
src/Microsoft.ML.Data/Transforms/Normalizer.cs86.03% <0%> (ø)⬆️
...icrosoft.ML.Functional.Tests/DataTransformation.cs100% <0%> (ø)⬆️
...s/Api/CookbookSamples/CookbookSamplesDynamicApi.cs93.49% <0%> (ø)⬆️
...s/Scenarios/Api/CookbookSamples/CookbookSamples.cs99.49% <0%> (ø)⬆️
test/Microsoft.ML.Functional.Tests/Training.cs100% <0%> (ø)⬆️
...est/Microsoft.ML.Tests/FeatureContributionTests.cs98.55% <0%> (ø)⬆️
...oft.ML.Experimental/TransformsCatalogExtensions.cs
... and 5 more

foreach (var row in data.RowView)
{
foreach (var kvPair in row.Values)
{

@shmoradimsshmoradimsApr 2, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

{ [](start = 16, length = 1)

for 1-liner blocks, let's not use {} to keep the samples short. similar to foreach loop on line 58 #Resolved

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

actually these are 2-liners. may use Linq to shorten it.


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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

keeping 2 for loops to keep logic simple


In reply to: 271453121 [](ancestors = 271453121,271452481)

private static void PrintPreview(DataDebuggerPreview data)
{
foreach (var colInfo in data.ColumnView)
Console.Write("{0, -25}", colInfo.Column.Name);

@shmoradimsshmoradimsApr 2, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

"{0, -25}" [](start = 30, length = 10)

let's just define this as:
var format = "{0,-25}" (they usually don't use space after comma)

and use 'format' in the rest of the code

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

not getting you .. whats meant by using 'format' ? is the suggestion to use string.Format somehow ..


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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

used control+k+d to fix format (if thats what u meant)


In reply to: 271462084 [](ancestors = 271462084,271455734)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ivan meant to define the "{0,-25}" as a variable, rather than repeating it, and reuse the variable below.


In reply to: 271465497 [](ancestors = 271465497,271462084,271455734)

var random = new Random(seed);

for (int i = 0; i < count; i++)
{

@shmoradimsshmoradimsApr 2, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

{ [](start = 12, length = 1)

omit #Resolved


private class DataPoint
{
[VectorType(inputSize)]

@sfilipisfilipiApr 3, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[VectorType(inputSize)] [](start = 11, length = 24)

is this annotation needed? #Resolved

@abgoswamabgoswamApr 3, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

yeah, this is indeed needed.

when fitting a model, we do some checks e.g. schema validation / size checks / whether the estimator works with fixed size or variable size etc

the VectorToImageConvertingEstimator works on fixed-size vector . without the annotation, the assumtion is that its a variable size vector , and the following check fails :

if(col.Kind!=SchemaShape.Column.VectorKind.Vector||(col.ItemType!=NumberDataViewType.Single&&col.ItemType!=NumberDataViewType.Double&&col.ItemType!=NumberDataViewType.Byte))

Unhandled Exception: System.ArgumentOutOfRangeException: Schema mismatch for input column 'Features': expected known-size vector of type Single, Double or Byte, got VarVector


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

@sfilipisfilipi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:shipit:

@abgoswam
abgoswam merged commit ac53748 into dotnet:masterApr 3, 2019
abgoswam added a commit to abgoswam/machinelearning that referenced this pull request Apr 5, 2019
…3165)
* updating image samples
* fix review comments (print preview)
* fix comments (minor nits)
* fix review comments (whitespaces)
@ghostghost locked as resolved and limited conversation to collaborators Mar 23, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@abgoswam@Ivanidzo4ka@shmoradims@sfilipi
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Samples and unit test for image-related transform estimators - #3165

Merged
abgoswam merged 4 commits into
dotnet:masterfrom
abgoswam:abgoswam/image_samples
Apr 3, 2019
Merged

Samples and unit test for image-related transform estimators#3165
abgoswam merged 4 commits into
dotnet:masterfrom
abgoswam:abgoswam/image_samples

Conversation

@abgoswam

Copy link
Copy Markdown
Member

Towards #1209

The PR makes the following changes

  • Adds unit test and sample for the ConvertToImage transform estimator.

  • Fixes the info presented to the user for the 4 existing image samples {ConvertToGrayscale, LoadImages, ExtractPixels, ResizeImages}

@abgoswamabgoswam changed the title Image samplesSamples and unit test for image-related transform estimatorsApr 2, 2019
@abgoswam
abgoswam requested a review from shmoradimsApril 2, 2019 01:32
// ImagePath : tomato.bmp
// Name : tomato
// ImageObject : System.Drawing.Bitmap
// Grayscale : System.Drawing.Bitmap

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe transpose, to give the column/row idea more accurately. #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

fixed


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

// Preview 1 row of the transformedData.
var transformedDataPreview = transformedData.Preview(1);
foreach (var kvPair in transformedDataPreview.RowView[0].Values)
{

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

{ [](start = 12, length = 1)

omit :) #Resolved

{
public static class ConvertToImage
{
private const int inputSize = 3 * 224 * 224;

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • 224 * 224; [](start = 39, length = 13)

might want to add a comment about what does each number represent #Resolved

Features = Enumerable.Repeat(0, inputSize).Select(x => random.NextDouble() * 100).ToArray()
};
}
}

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i'd move this below the example. If the users want to scroll to it they can. #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

fixed


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

// Preview 1 row of the transformedData.
var transformedDataPreview = transformedData.Preview(1);
foreach (var kvPair in transformedDataPreview.RowView[0].Values)
{

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

{ [](start = 11, length = 2)

omit #Resolved


// The transformedData IDataView contains the loaded and resized raw values

// Preview 1 row of the transformedData.

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no need for space in between. #Resolved

// Pixels : Dense vector of size 30000

Console.WriteLine("--------------------------------------------------");

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no need for it. Users can add it #Resolved

Console.WriteLine("--------------------------------------------------");

// Using schema comprehension to display raw pixels for each row.
// Display extracted pixels in column 'Pixels'.

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

might be easier to understand if put as:

Convert the transformedData into an IEnumerable #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

modified the pretty printing method


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


// Preview of the content of the images.tsv file
//
// ImagePath Name ImageObject "Pixels"

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

// ImagePath Name ImageObject [](start = 11, length = 41)

maybe keep the headers/column names, even if they don't print out. Or print them separately. #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

fixed


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

var pipeline = mlContext.Transforms.LoadImages("ImageObject", imagesFolder, "ImagePath");

var transformedData = pipeline.Fit(data).Transform(data);
// The transformedData IDataView contains the loaded images now

@Ivanidzo4kaIvanidzo4kaApr 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

now [](start = 72, length = 3)

dot in the end of sentence :) #Resolved

.Append(mlContext.Transforms.ResizeImages("ImageObjectResized", inputColumnName: "ImageObject", imageWidth: 100, imageHeight: 100));

var transformedData = pipeline.Fit(data).Transform(data);
// The transformedData IDataView contains the resized images now

@Ivanidzo4kaIvanidzo4kaApr 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

dot here is well #Resolved

@Ivanidzo4kaIvanidzo4ka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:shipit:

@codecov

codecovBot commented Apr 2, 2019

Copy link
Copy Markdown

Codecov Report

Merging #3165 into master will increase coverage by 0.01%.
The diff coverage is 94.73%.

@@ Coverage Diff @@## master #3165 +/- ##
==========================================
+ Coverage 72.53% 72.55% +0.01% 
==========================================
Files 808 807 -1 Lines 144775 144793 +18 Branches 16209 16211 +2 ==========================================
+ Hits 105012 105054 +42 + Misses 35348 35323 -25 - Partials 4415 4416 +1
FlagCoverage Δ
#Debug72.55% <94.73%> (+0.01%)⬆️
#production68.14% <ø> (+0.02%)⬆️
#test88.83% <94.73%> (ø)⬆️
Impacted FilesCoverage Δ
...c/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs16.66% <ø> (+5.55%)⬆️
test/Microsoft.ML.Tests/ImagesTests.cs98.69% <94.73%> (-0.13%)⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.1% <0%> (-0.16%)⬇️
src/Microsoft.ML.Data/Transforms/Normalizer.cs86.03% <0%> (ø)⬆️
...icrosoft.ML.Functional.Tests/DataTransformation.cs100% <0%> (ø)⬆️
...s/Api/CookbookSamples/CookbookSamplesDynamicApi.cs93.49% <0%> (ø)⬆️
...s/Scenarios/Api/CookbookSamples/CookbookSamples.cs99.49% <0%> (ø)⬆️
test/Microsoft.ML.Functional.Tests/Training.cs100% <0%> (ø)⬆️
...est/Microsoft.ML.Tests/FeatureContributionTests.cs98.55% <0%> (ø)⬆️
...oft.ML.Experimental/TransformsCatalogExtensions.cs
... and 5 more

@codecov

codecovBot commented Apr 2, 2019

Copy link
Copy Markdown

Codecov Report

Merging #3165 into master will increase coverage by 0.01%.
The diff coverage is 94.73%.

@@ Coverage Diff @@## master #3165 +/- ##
==========================================
+ Coverage 72.53% 72.55% +0.01% 
==========================================
Files 808 807 -1 Lines 144775 144793 +18 Branches 16209 16211 +2 ==========================================
+ Hits 105012 105054 +42 + Misses 35348 35322 -26 - Partials 4415 4417 +2
FlagCoverage Δ
#Debug72.55% <94.73%> (+0.01%)⬆️
#production68.14% <ø> (+0.02%)⬆️
#test88.83% <94.73%> (ø)⬆️
Impacted FilesCoverage Δ
...c/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs16.66% <ø> (+5.55%)⬆️
test/Microsoft.ML.Tests/ImagesTests.cs98.69% <94.73%> (-0.13%)⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.1% <0%> (-0.16%)⬇️
src/Microsoft.ML.Data/Transforms/Normalizer.cs86.03% <0%> (ø)⬆️
...icrosoft.ML.Functional.Tests/DataTransformation.cs100% <0%> (ø)⬆️
...s/Api/CookbookSamples/CookbookSamplesDynamicApi.cs93.49% <0%> (ø)⬆️
...s/Scenarios/Api/CookbookSamples/CookbookSamples.cs99.49% <0%> (ø)⬆️
test/Microsoft.ML.Functional.Tests/Training.cs100% <0%> (ø)⬆️
...est/Microsoft.ML.Tests/FeatureContributionTests.cs98.55% <0%> (ø)⬆️
...oft.ML.Experimental/TransformsCatalogExtensions.cs
... and 5 more

foreach (var row in data.RowView)
{
foreach (var kvPair in row.Values)
{

@shmoradimsshmoradimsApr 2, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

{ [](start = 16, length = 1)

for 1-liner blocks, let's not use {} to keep the samples short. similar to foreach loop on line 58 #Resolved

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

actually these are 2-liners. may use Linq to shorten it.


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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

keeping 2 for loops to keep logic simple


In reply to: 271453121 [](ancestors = 271453121,271452481)

private static void PrintPreview(DataDebuggerPreview data)
{
foreach (var colInfo in data.ColumnView)
Console.Write("{0, -25}", colInfo.Column.Name);

@shmoradimsshmoradimsApr 2, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

"{0, -25}" [](start = 30, length = 10)

let's just define this as:
var format = "{0,-25}" (they usually don't use space after comma)

and use 'format' in the rest of the code

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

not getting you .. whats meant by using 'format' ? is the suggestion to use string.Format somehow ..


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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

used control+k+d to fix format (if thats what u meant)


In reply to: 271462084 [](ancestors = 271462084,271455734)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ivan meant to define the "{0,-25}" as a variable, rather than repeating it, and reuse the variable below.


In reply to: 271465497 [](ancestors = 271465497,271462084,271455734)

var random = new Random(seed);

for (int i = 0; i < count; i++)
{

@shmoradimsshmoradimsApr 2, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

{ [](start = 12, length = 1)

omit #Resolved


private class DataPoint
{
[VectorType(inputSize)]

@sfilipisfilipiApr 3, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[VectorType(inputSize)] [](start = 11, length = 24)

is this annotation needed? #Resolved

@abgoswamabgoswamApr 3, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

yeah, this is indeed needed.

when fitting a model, we do some checks e.g. schema validation / size checks / whether the estimator works with fixed size or variable size etc

the VectorToImageConvertingEstimator works on fixed-size vector . without the annotation, the assumtion is that its a variable size vector , and the following check fails :

if(col.Kind!=SchemaShape.Column.VectorKind.Vector||(col.ItemType!=NumberDataViewType.Single&&col.ItemType!=NumberDataViewType.Double&&col.ItemType!=NumberDataViewType.Byte))

Unhandled Exception: System.ArgumentOutOfRangeException: Schema mismatch for input column 'Features': expected known-size vector of type Single, Double or Byte, got VarVector


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

@sfilipisfilipi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:shipit:

@abgoswam
abgoswam merged commit ac53748 into dotnet:masterApr 3, 2019
abgoswam added a commit to abgoswam/machinelearning that referenced this pull request Apr 5, 2019
…3165)
* updating image samples
* fix review comments (print preview)
* fix comments (minor nits)
* fix review comments (whitespaces)
@ghostghost locked as resolved and limited conversation to collaborators Mar 23, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@abgoswam@Ivanidzo4ka@shmoradims@sfilipi
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Samples and unit test for image-related transform estimators - #3165

Merged
abgoswam merged 4 commits into
dotnet:masterfrom
abgoswam:abgoswam/image_samples
Apr 3, 2019
Merged

Samples and unit test for image-related transform estimators#3165
abgoswam merged 4 commits into
dotnet:masterfrom
abgoswam:abgoswam/image_samples

Conversation

@abgoswam

Copy link
Copy Markdown
Member

Towards #1209

The PR makes the following changes

  • Adds unit test and sample for the ConvertToImage transform estimator.

  • Fixes the info presented to the user for the 4 existing image samples {ConvertToGrayscale, LoadImages, ExtractPixels, ResizeImages}

@abgoswamabgoswam changed the title Image samplesSamples and unit test for image-related transform estimatorsApr 2, 2019
@abgoswam
abgoswam requested a review from shmoradimsApril 2, 2019 01:32
// ImagePath : tomato.bmp
// Name : tomato
// ImageObject : System.Drawing.Bitmap
// Grayscale : System.Drawing.Bitmap

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe transpose, to give the column/row idea more accurately. #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

fixed


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

// Preview 1 row of the transformedData.
var transformedDataPreview = transformedData.Preview(1);
foreach (var kvPair in transformedDataPreview.RowView[0].Values)
{

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

{ [](start = 12, length = 1)

omit :) #Resolved

{
public static class ConvertToImage
{
private const int inputSize = 3 * 224 * 224;

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • 224 * 224; [](start = 39, length = 13)

might want to add a comment about what does each number represent #Resolved

Features = Enumerable.Repeat(0, inputSize).Select(x => random.NextDouble() * 100).ToArray()
};
}
}

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i'd move this below the example. If the users want to scroll to it they can. #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

fixed


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

// Preview 1 row of the transformedData.
var transformedDataPreview = transformedData.Preview(1);
foreach (var kvPair in transformedDataPreview.RowView[0].Values)
{

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

{ [](start = 11, length = 2)

omit #Resolved


// The transformedData IDataView contains the loaded and resized raw values

// Preview 1 row of the transformedData.

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no need for space in between. #Resolved

// Pixels : Dense vector of size 30000

Console.WriteLine("--------------------------------------------------");

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no need for it. Users can add it #Resolved

Console.WriteLine("--------------------------------------------------");

// Using schema comprehension to display raw pixels for each row.
// Display extracted pixels in column 'Pixels'.

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

might be easier to understand if put as:

Convert the transformedData into an IEnumerable #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

modified the pretty printing method


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


// Preview of the content of the images.tsv file
//
// ImagePath Name ImageObject "Pixels"

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

// ImagePath Name ImageObject [](start = 11, length = 41)

maybe keep the headers/column names, even if they don't print out. Or print them separately. #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

fixed


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

var pipeline = mlContext.Transforms.LoadImages("ImageObject", imagesFolder, "ImagePath");

var transformedData = pipeline.Fit(data).Transform(data);
// The transformedData IDataView contains the loaded images now

@Ivanidzo4kaIvanidzo4kaApr 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

now [](start = 72, length = 3)

dot in the end of sentence :) #Resolved

.Append(mlContext.Transforms.ResizeImages("ImageObjectResized", inputColumnName: "ImageObject", imageWidth: 100, imageHeight: 100));

var transformedData = pipeline.Fit(data).Transform(data);
// The transformedData IDataView contains the resized images now

@Ivanidzo4kaIvanidzo4kaApr 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

dot here is well #Resolved

@Ivanidzo4kaIvanidzo4ka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:shipit:

@codecov

codecovBot commented Apr 2, 2019

Copy link
Copy Markdown

Codecov Report

Merging #3165 into master will increase coverage by 0.01%.
The diff coverage is 94.73%.

@@ Coverage Diff @@## master #3165 +/- ##
==========================================
+ Coverage 72.53% 72.55% +0.01% 
==========================================
Files 808 807 -1 Lines 144775 144793 +18 Branches 16209 16211 +2 ==========================================
+ Hits 105012 105054 +42 + Misses 35348 35323 -25 - Partials 4415 4416 +1
FlagCoverage Δ
#Debug72.55% <94.73%> (+0.01%)⬆️
#production68.14% <ø> (+0.02%)⬆️
#test88.83% <94.73%> (ø)⬆️
Impacted FilesCoverage Δ
...c/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs16.66% <ø> (+5.55%)⬆️
test/Microsoft.ML.Tests/ImagesTests.cs98.69% <94.73%> (-0.13%)⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.1% <0%> (-0.16%)⬇️
src/Microsoft.ML.Data/Transforms/Normalizer.cs86.03% <0%> (ø)⬆️
...icrosoft.ML.Functional.Tests/DataTransformation.cs100% <0%> (ø)⬆️
...s/Api/CookbookSamples/CookbookSamplesDynamicApi.cs93.49% <0%> (ø)⬆️
...s/Scenarios/Api/CookbookSamples/CookbookSamples.cs99.49% <0%> (ø)⬆️
test/Microsoft.ML.Functional.Tests/Training.cs100% <0%> (ø)⬆️
...est/Microsoft.ML.Tests/FeatureContributionTests.cs98.55% <0%> (ø)⬆️
...oft.ML.Experimental/TransformsCatalogExtensions.cs
... and 5 more

@codecov

codecovBot commented Apr 2, 2019

Copy link
Copy Markdown

Codecov Report

Merging #3165 into master will increase coverage by 0.01%.
The diff coverage is 94.73%.

@@ Coverage Diff @@## master #3165 +/- ##
==========================================
+ Coverage 72.53% 72.55% +0.01% 
==========================================
Files 808 807 -1 Lines 144775 144793 +18 Branches 16209 16211 +2 ==========================================
+ Hits 105012 105054 +42 + Misses 35348 35322 -26 - Partials 4415 4417 +2
FlagCoverage Δ
#Debug72.55% <94.73%> (+0.01%)⬆️
#production68.14% <ø> (+0.02%)⬆️
#test88.83% <94.73%> (ø)⬆️
Impacted FilesCoverage Δ
...c/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs16.66% <ø> (+5.55%)⬆️
test/Microsoft.ML.Tests/ImagesTests.cs98.69% <94.73%> (-0.13%)⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.1% <0%> (-0.16%)⬇️
src/Microsoft.ML.Data/Transforms/Normalizer.cs86.03% <0%> (ø)⬆️
...icrosoft.ML.Functional.Tests/DataTransformation.cs100% <0%> (ø)⬆️
...s/Api/CookbookSamples/CookbookSamplesDynamicApi.cs93.49% <0%> (ø)⬆️
...s/Scenarios/Api/CookbookSamples/CookbookSamples.cs99.49% <0%> (ø)⬆️
test/Microsoft.ML.Functional.Tests/Training.cs100% <0%> (ø)⬆️
...est/Microsoft.ML.Tests/FeatureContributionTests.cs98.55% <0%> (ø)⬆️
...oft.ML.Experimental/TransformsCatalogExtensions.cs
... and 5 more

foreach (var row in data.RowView)
{
foreach (var kvPair in row.Values)
{

@shmoradimsshmoradimsApr 2, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

{ [](start = 16, length = 1)

for 1-liner blocks, let's not use {} to keep the samples short. similar to foreach loop on line 58 #Resolved

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

actually these are 2-liners. may use Linq to shorten it.


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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

keeping 2 for loops to keep logic simple


In reply to: 271453121 [](ancestors = 271453121,271452481)

private static void PrintPreview(DataDebuggerPreview data)
{
foreach (var colInfo in data.ColumnView)
Console.Write("{0, -25}", colInfo.Column.Name);

@shmoradimsshmoradimsApr 2, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

"{0, -25}" [](start = 30, length = 10)

let's just define this as:
var format = "{0,-25}" (they usually don't use space after comma)

and use 'format' in the rest of the code

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

not getting you .. whats meant by using 'format' ? is the suggestion to use string.Format somehow ..


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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

used control+k+d to fix format (if thats what u meant)


In reply to: 271462084 [](ancestors = 271462084,271455734)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ivan meant to define the "{0,-25}" as a variable, rather than repeating it, and reuse the variable below.


In reply to: 271465497 [](ancestors = 271465497,271462084,271455734)

var random = new Random(seed);

for (int i = 0; i < count; i++)
{

@shmoradimsshmoradimsApr 2, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

{ [](start = 12, length = 1)

omit #Resolved


private class DataPoint
{
[VectorType(inputSize)]

@sfilipisfilipiApr 3, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[VectorType(inputSize)] [](start = 11, length = 24)

is this annotation needed? #Resolved

@abgoswamabgoswamApr 3, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

yeah, this is indeed needed.

when fitting a model, we do some checks e.g. schema validation / size checks / whether the estimator works with fixed size or variable size etc

the VectorToImageConvertingEstimator works on fixed-size vector . without the annotation, the assumtion is that its a variable size vector , and the following check fails :

if(col.Kind!=SchemaShape.Column.VectorKind.Vector||(col.ItemType!=NumberDataViewType.Single&&col.ItemType!=NumberDataViewType.Double&&col.ItemType!=NumberDataViewType.Byte))

Unhandled Exception: System.ArgumentOutOfRangeException: Schema mismatch for input column 'Features': expected known-size vector of type Single, Double or Byte, got VarVector


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

@sfilipisfilipi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:shipit:

@abgoswam
abgoswam merged commit ac53748 into dotnet:masterApr 3, 2019
abgoswam added a commit to abgoswam/machinelearning that referenced this pull request Apr 5, 2019
…3165)
* updating image samples
* fix review comments (print preview)
* fix comments (minor nits)
* fix review comments (whitespaces)
@ghostghost locked as resolved and limited conversation to collaborators Mar 23, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@abgoswam@Ivanidzo4ka@shmoradims@sfilipi
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Samples and unit test for image-related transform estimators - #3165

Merged
abgoswam merged 4 commits into
dotnet:masterfrom
abgoswam:abgoswam/image_samples
Apr 3, 2019
Merged

Samples and unit test for image-related transform estimators#3165
abgoswam merged 4 commits into
dotnet:masterfrom
abgoswam:abgoswam/image_samples

Conversation

@abgoswam

Copy link
Copy Markdown
Member

Towards #1209

The PR makes the following changes

  • Adds unit test and sample for the ConvertToImage transform estimator.

  • Fixes the info presented to the user for the 4 existing image samples {ConvertToGrayscale, LoadImages, ExtractPixels, ResizeImages}

@abgoswamabgoswam changed the title Image samplesSamples and unit test for image-related transform estimatorsApr 2, 2019
@abgoswam
abgoswam requested a review from shmoradimsApril 2, 2019 01:32
// ImagePath : tomato.bmp
// Name : tomato
// ImageObject : System.Drawing.Bitmap
// Grayscale : System.Drawing.Bitmap

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe transpose, to give the column/row idea more accurately. #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

fixed


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

// Preview 1 row of the transformedData.
var transformedDataPreview = transformedData.Preview(1);
foreach (var kvPair in transformedDataPreview.RowView[0].Values)
{

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

{ [](start = 12, length = 1)

omit :) #Resolved

{
public static class ConvertToImage
{
private const int inputSize = 3 * 224 * 224;

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • 224 * 224; [](start = 39, length = 13)

might want to add a comment about what does each number represent #Resolved

Features = Enumerable.Repeat(0, inputSize).Select(x => random.NextDouble() * 100).ToArray()
};
}
}

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i'd move this below the example. If the users want to scroll to it they can. #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

fixed


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

// Preview 1 row of the transformedData.
var transformedDataPreview = transformedData.Preview(1);
foreach (var kvPair in transformedDataPreview.RowView[0].Values)
{

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

{ [](start = 11, length = 2)

omit #Resolved


// The transformedData IDataView contains the loaded and resized raw values

// Preview 1 row of the transformedData.

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no need for space in between. #Resolved

// Pixels : Dense vector of size 30000

Console.WriteLine("--------------------------------------------------");

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no need for it. Users can add it #Resolved

Console.WriteLine("--------------------------------------------------");

// Using schema comprehension to display raw pixels for each row.
// Display extracted pixels in column 'Pixels'.

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

might be easier to understand if put as:

Convert the transformedData into an IEnumerable #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

modified the pretty printing method


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


// Preview of the content of the images.tsv file
//
// ImagePath Name ImageObject "Pixels"

@sfilipisfilipiApr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

// ImagePath Name ImageObject [](start = 11, length = 41)

maybe keep the headers/column names, even if they don't print out. Or print them separately. #Resolved

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

fixed


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

var pipeline = mlContext.Transforms.LoadImages("ImageObject", imagesFolder, "ImagePath");

var transformedData = pipeline.Fit(data).Transform(data);
// The transformedData IDataView contains the loaded images now

@Ivanidzo4kaIvanidzo4kaApr 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

now [](start = 72, length = 3)

dot in the end of sentence :) #Resolved

.Append(mlContext.Transforms.ResizeImages("ImageObjectResized", inputColumnName: "ImageObject", imageWidth: 100, imageHeight: 100));

var transformedData = pipeline.Fit(data).Transform(data);
// The transformedData IDataView contains the resized images now

@Ivanidzo4kaIvanidzo4kaApr 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

dot here is well #Resolved

@Ivanidzo4kaIvanidzo4ka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:shipit:

@codecov

codecovBot commented Apr 2, 2019

Copy link
Copy Markdown

Codecov Report

Merging #3165 into master will increase coverage by 0.01%.
The diff coverage is 94.73%.

@@ Coverage Diff @@## master #3165 +/- ##
==========================================
+ Coverage 72.53% 72.55% +0.01% 
==========================================
Files 808 807 -1 Lines 144775 144793 +18 Branches 16209 16211 +2 ==========================================
+ Hits 105012 105054 +42 + Misses 35348 35323 -25 - Partials 4415 4416 +1
FlagCoverage Δ
#Debug72.55% <94.73%> (+0.01%)⬆️
#production68.14% <ø> (+0.02%)⬆️
#test88.83% <94.73%> (ø)⬆️
Impacted FilesCoverage Δ
...c/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs16.66% <ø> (+5.55%)⬆️
test/Microsoft.ML.Tests/ImagesTests.cs98.69% <94.73%> (-0.13%)⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.1% <0%> (-0.16%)⬇️
src/Microsoft.ML.Data/Transforms/Normalizer.cs86.03% <0%> (ø)⬆️
...icrosoft.ML.Functional.Tests/DataTransformation.cs100% <0%> (ø)⬆️
...s/Api/CookbookSamples/CookbookSamplesDynamicApi.cs93.49% <0%> (ø)⬆️
...s/Scenarios/Api/CookbookSamples/CookbookSamples.cs99.49% <0%> (ø)⬆️
test/Microsoft.ML.Functional.Tests/Training.cs100% <0%> (ø)⬆️
...est/Microsoft.ML.Tests/FeatureContributionTests.cs98.55% <0%> (ø)⬆️
...oft.ML.Experimental/TransformsCatalogExtensions.cs
... and 5 more

@codecov

codecovBot commented Apr 2, 2019

Copy link
Copy Markdown

Codecov Report

Merging #3165 into master will increase coverage by 0.01%.
The diff coverage is 94.73%.

@@ Coverage Diff @@## master #3165 +/- ##
==========================================
+ Coverage 72.53% 72.55% +0.01% 
==========================================
Files 808 807 -1 Lines 144775 144793 +18 Branches 16209 16211 +2 ==========================================
+ Hits 105012 105054 +42 + Misses 35348 35322 -26 - Partials 4415 4417 +2
FlagCoverage Δ
#Debug72.55% <94.73%> (+0.01%)⬆️
#production68.14% <ø> (+0.02%)⬆️
#test88.83% <94.73%> (ø)⬆️
Impacted FilesCoverage Δ
...c/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs16.66% <ø> (+5.55%)⬆️
test/Microsoft.ML.Tests/ImagesTests.cs98.69% <94.73%> (-0.13%)⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.1% <0%> (-0.16%)⬇️
src/Microsoft.ML.Data/Transforms/Normalizer.cs86.03% <0%> (ø)⬆️
...icrosoft.ML.Functional.Tests/DataTransformation.cs100% <0%> (ø)⬆️
...s/Api/CookbookSamples/CookbookSamplesDynamicApi.cs93.49% <0%> (ø)⬆️
...s/Scenarios/Api/CookbookSamples/CookbookSamples.cs99.49% <0%> (ø)⬆️
test/Microsoft.ML.Functional.Tests/Training.cs100% <0%> (ø)⬆️
...est/Microsoft.ML.Tests/FeatureContributionTests.cs98.55% <0%> (ø)⬆️
...oft.ML.Experimental/TransformsCatalogExtensions.cs
... and 5 more

foreach (var row in data.RowView)
{
foreach (var kvPair in row.Values)
{

@shmoradimsshmoradimsApr 2, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

{ [](start = 16, length = 1)

for 1-liner blocks, let's not use {} to keep the samples short. similar to foreach loop on line 58 #Resolved

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

actually these are 2-liners. may use Linq to shorten it.


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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

keeping 2 for loops to keep logic simple


In reply to: 271453121 [](ancestors = 271453121,271452481)

private static void PrintPreview(DataDebuggerPreview data)
{
foreach (var colInfo in data.ColumnView)
Console.Write("{0, -25}", colInfo.Column.Name);

@shmoradimsshmoradimsApr 2, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

"{0, -25}" [](start = 30, length = 10)

let's just define this as:
var format = "{0,-25}" (they usually don't use space after comma)

and use 'format' in the rest of the code

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

not getting you .. whats meant by using 'format' ? is the suggestion to use string.Format somehow ..


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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

used control+k+d to fix format (if thats what u meant)


In reply to: 271462084 [](ancestors = 271462084,271455734)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ivan meant to define the "{0,-25}" as a variable, rather than repeating it, and reuse the variable below.


In reply to: 271465497 [](ancestors = 271465497,271462084,271455734)

var random = new Random(seed);

for (int i = 0; i < count; i++)
{

@shmoradimsshmoradimsApr 2, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

{ [](start = 12, length = 1)

omit #Resolved


private class DataPoint
{
[VectorType(inputSize)]

@sfilipisfilipiApr 3, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[VectorType(inputSize)] [](start = 11, length = 24)

is this annotation needed? #Resolved

@abgoswamabgoswamApr 3, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

yeah, this is indeed needed.

when fitting a model, we do some checks e.g. schema validation / size checks / whether the estimator works with fixed size or variable size etc

the VectorToImageConvertingEstimator works on fixed-size vector . without the annotation, the assumtion is that its a variable size vector , and the following check fails :

if(col.Kind!=SchemaShape.Column.VectorKind.Vector||(col.ItemType!=NumberDataViewType.Single&&col.ItemType!=NumberDataViewType.Double&&col.ItemType!=NumberDataViewType.Byte))

Unhandled Exception: System.ArgumentOutOfRangeException: Schema mismatch for input column 'Features': expected known-size vector of type Single, Double or Byte, got VarVector


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

@sfilipisfilipi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:shipit:

@abgoswam
abgoswam merged commit ac53748 into dotnet:masterApr 3, 2019
abgoswam added a commit to abgoswam/machinelearning that referenced this pull request Apr 5, 2019
…3165)
* updating image samples
* fix review comments (print preview)
* fix comments (minor nits)
* fix review comments (whitespaces)
@ghostghost locked as resolved and limited conversation to collaborators Mar 23, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@abgoswam@Ivanidzo4ka@shmoradims@sfilipi