Fix a value-mapping bug - #3180

Merged
wschin merged 4 commits into
dotnet:masterfrom
wschin:fix-value-map
Apr 4, 2019
Merged

Fix a value-mapping bug#3180
wschin merged 4 commits into
dotnet:masterfrom
wschin:fix-value-map

Conversation

@wschin

@wschinwschin commented Apr 2, 2019

Copy link
Copy Markdown
Contributor

This PR fixes#3166.

@wschinwschin added the bug Something isn't working label Apr 2, 2019
@wschinwschin self-assigned this Apr 2, 2019
@wschin
wschin requested review from sfilipi and singlisApril 2, 2019 21:43
@codecov

codecovBot commented Apr 2, 2019•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #3180 into master will increase coverage by 0.06%.
The diff coverage is 97.19%.

@@ Coverage Diff @@## master #3180 +/- ##
==========================================
+ Coverage 72.54% 72.6% +0.06% 
==========================================
Files 807 807 Lines 144774 145077 +303 Branches 16208 16213 +5 ==========================================
+ Hits 105021 105335 +314 + Misses 35339 35324 -15 - Partials 4414 4418 +4
FlagCoverage Δ
#Debug72.6% <97.19%> (+0.06%)⬆️
#production68.15% <91.42%> (+0.02%)⬆️
#test88.92% <100%> (+0.09%)⬆️
Impacted FilesCoverage Δ
...crosoft.ML.Tests/Transformers/ValueMappingTests.cs100% <100%> (ø)⬆️
src/Microsoft.ML.Data/Transforms/ValueMapping.cs84.26% <91.22%> (-0.14%)⬇️
...ML.Data/Transforms/ConversionsExtensionsCatalog.cs64.07% <92.3%> (+19.2%)⬆️
...c/Microsoft.ML.FastTree/Utils/ThreadTaskManager.cs79.48% <0%> (-20.52%)⬇️
src/Microsoft.ML.DataView/KeyDataViewType.cs74.57% <0%> (-3.76%)⬇️
src/Microsoft.ML.Maml/MAML.cs24.75% <0%> (-1.46%)⬇️
test/Microsoft.ML.Tests/ImagesTests.cs98.69% <0%> (-0.13%)⬇️
...Microsoft.ML.Tests/Transformers/NormalizerTests.cs100% <0%> (ø)⬆️
src/Microsoft.ML.Transforms/Text/TextCatalog.cs41.66% <0%> (ø)⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.26% <0%> (+0.15%)⬆️
... and 5 more

singlis
singlis reviewed Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
/// <param name="keyColumn">Name of the key column in <paramref name="lookupMap"/>.</param>
/// <param name="valueColumn">Name of the value column in <paramref name="lookupMap"/>.</param>
/// <param name="columns">The list of names of the input columns to apply the transformation, and the name of the resulting column.</param>
internal ValueMappingEstimator(IHostEnvironment env, IDataView lookupMap, DataViewSchema.Column keyColumn, DataViewSchema.Column valueColumn, params (string outputColumnName, string inputColumnName)[] columns)

@singlissinglisApr 2, 2019•
edited by wschin
Loading

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.

If we are adding this constructor, is the one above (that takes a key column name and value column name) needed? #Resolved

@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.

+1, especially since the extension that calls into the ctor, takes in DataViewSchema.Column and passes down only the name of the column.

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

@wschinwschinApr 4, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I will remove it in the next iteration.

In reply to: 271577343 [](ancestors = 271577343,271533565)

singlis
singlis reviewed Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs Outdated
var valueType = dataView.Schema[valueIdx].Type;
var valueMap = ValueMap.Create(keyType, valueType, ValueColumnMetadata);
using (var cursor = dataView.GetRowCursor(dataView.Schema[keyIdx], dataView.Schema[valueIdx]))
var retrievedKeyColumn = dataView.Schema[keyColumn.Index];

@singlissinglisApr 2, 2019•
edited by wschin
Loading

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.

Schema [](start = 46, length = 6)

Can this be out of range? #Resolved

@wschinwschinApr 3, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Will add a check in the next iteration.

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

singlis
singlis reviewed Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs Outdated
Host.Check(retrievedKeyColumn.Type == keyColumn.Type, nameof(keyColumn) + "'s column type doesn't match that of the associated column in " + nameof(dataView));
Host.Check(retrievedKeyColumn.Annotations == keyColumn.Annotations, nameof(keyColumn) + "'s column annotions don't match those of the associated column in " + nameof(dataView));

var retrievedValueColumn = dataView.Schema[valueColumn.Index];

@singlissinglisApr 2, 2019•
edited by wschin
Loading

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.

same here. #Resolved

@wschinwschinApr 3, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Will add a check as well.

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

singlis
singlis reviewed Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs Outdated
private readonly byte[] _dataView;

private readonly DataViewSchema.Column _lookupKeyColumn;
private readonly DataViewSchema.Column _lookupValueColumn;

@singlissinglisApr 2, 2019•
edited by wschin
Loading

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.

private [](start = 8, length = 7)

Why store these? They look like they can be removed. #Resolved

@wschinwschinApr 4, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Super nice catch. Thanks.

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

singlis
singlis approved these changes Apr 2, 2019
View reviewed changes

@singlissinglis 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:

Ivanidzo4ka
Ivanidzo4ka reviewed Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs Outdated
@@ -46,7 +46,22 @@ public class ValueMappingEstimator : TrivialEstimator<ValueMappingTransformer>
/// <param name="columns">The list of names of the input columns to apply the transformation, and the name of the resulting column.</param>
internal ValueMappingEstimator(IHostEnvironment env, IDataView lookupMap, string keyColumn, string valueColumn, params (string outputColumnName, string inputColumnName)[] columns)

@Ivanidzo4kaIvanidzo4kaApr 3, 2019•
edited by sfilipi
Loading

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.

ValueMappingEstimator [](start = 17, length = 21)

I'm not getting why we still have this constructor
Public interface operates over Schema.Column object, any conversion from string to Schema.Column is unrelaable, why it's still around? #Closed

@wschinwschinApr 4, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Removed and related code is updated as well.

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

sfilipi
sfilipi reviewed Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
DataViewSchema.Column lookupKeyColumn, DataViewSchema.Column lookupValueColumn, (string outputColumnName, string inputColumnName)[] columns)
: base(Contracts.CheckRef(env, nameof(env)).Register(nameof(ValueMappingTransformer)), columns)
{
Host.CheckNonEmpty(keyColumn, nameof(keyColumn), "A key column must be specified when passing in an IDataView for the value mapping");

@sfilipisfilipiApr 3, 2019•
edited
Loading

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.

Host.CheckNonEmpty [](start = 11, length = 19)

maybe check lookupKeyColumn.HasValue #Closed

@wschinwschinApr 3, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

We can't. This is a struct, not a class.

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

sfilipi
sfilipi reviewed Apr 3, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/Transformers/ValueMappingTests.cs Outdated
// Create a new ML context, for ML.NET operations. It can be used for exception tracking and logging,
// as well as the source of randomness.
var mlContext = new MLContext();

@sfilipisfilipiApr 3, 2019•
edited
Loading

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.

you don't need this, the file should already have it: ML #Closed

sfilipi
sfilipi reviewed Apr 3, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/Transformers/ValueMappingTests.cs Outdated
// Getting the resulting data as an IEnumerable.
var features = mlContext.Data.CreateEnumerable<TransformedData>(transformedData, reuseRowObject: false).ToList();

var expectedPrices = new float[] { 3.14f, 2000f, 1.19f, 2.17f, 33.784f };

@sfilipisfilipiApr 3, 2019•
edited by wschin
Loading

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.

expectedPrices [](start = 16, length = 14)

you have rawData storing those #Resolved

@wschinwschinApr 4, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes. Thank you.

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

sfilipi
sfilipi reviewed Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
_lookupValueColumn = lookupValueColumn;
_valueMap = CreateValueMapFromDataView(lookupMap, _lookupKeyColumn, _lookupValueColumn);
ValueColumnMetadata = lookupMap.Schema[_lookupValueColumn.Index].Annotations;

@sfilipisfilipiApr 3, 2019•
edited by wschin
Loading

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.

Does _lookupValueColumn.Annotations work? #Resolved

@wschinwschinApr 4, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Just did a small refactorization around Annotations and extent a test to cover it at Iteration 4.

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

sfilipi
sfilipi reviewed Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs Outdated
var retrievedKeyColumn = dataView.Schema[keyColumn.Index];
Host.Check(retrievedKeyColumn.Index == keyColumn.Index, nameof(keyColumn) + "'s column index doesn't match that of the associated column in " + nameof(dataView));
Host.Check(retrievedKeyColumn.Type == keyColumn.Type, nameof(keyColumn) + "'s column type doesn't match that of the associated column in " + nameof(dataView));
Host.Check(retrievedKeyColumn.Annotations == keyColumn.Annotations, nameof(keyColumn) + "'s column annotions don't match those of the associated column in " + nameof(dataView));

@sfilipisfilipiApr 3, 2019•
edited by wschin
Loading

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.

annotions [](start = 111, length = 9)

typo #Resolved

sfilipi
sfilipi reviewed Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs Outdated
var valueType = dataView.Schema[valueIdx].Type;
var valueMap = ValueMap.Create(keyType, valueType, ValueColumnMetadata);
using (var cursor = dataView.GetRowCursor(dataView.Schema[keyIdx], dataView.Schema[valueIdx]))
var retrievedKeyColumn = dataView.Schema[keyColumn.Index];

@sfilipisfilipiApr 3, 2019•
edited by wschin
Loading

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.

retrievedKeyColumn [](start = 16, length = 18)

do we have any overload for Equals on the DataViewSchema.Column? The checks below seem like they would belong in that Equals. #Resolved

@wschinwschinApr 3, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

We don't.

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

@sfilipisfilipiApr 4, 2019•
edited
Loading

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'll open a bug about it.. we should have it.

In reply to: 271966741 [](ancestors = 271966741,271578862)

sfilipi
sfilipi reviewed Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs Outdated
/// </summary>
private abstract class ValueMap
{
public readonly string KeyColumnName;

@sfilipisfilipiApr 3, 2019•
edited by wschin
Loading

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.

KeyColumnName [](start = 35, length = 13)

what do you think about using the entire column, rather than the name? we have a mix of storing indices, names, columns.. #Resolved

@wschinwschinApr 3, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I will merge Name and Type into DataViewSchema.Column and clean redundant functions up.

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

sfilipi
sfilipi approved these changes Apr 4, 2019
View reviewed changes

@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:

sfilipi
sfilipi reviewed Apr 4, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ConversionsExtensionsCatalog.cs
return new ValueMappingEstimator<TInputType, TOutputType>(catalog.GetEnvironment(), lookupMap,
lookupMap.Schema[ValueMappingTransformer.DefaultKeyColumnName],
lookupMap.Schema[ValueMappingTransformer.DefaultValueColumnName],
InputOutputColumnPair.ConvertToValueTuples(columns));

@sfilipisfilipiApr 4, 2019•
edited
Loading

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.

there is a bit of repetition here, does it make sense to make it part of the ctor? I mean create the lookup map inside the ctor, just pass the columns.

@wschinwschinApr 4, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I feel columns must go with the associated IDataView. There should be no column if its IDataView hasn't been created.

@wschin
wschin merged commit b8a70ac into dotnet:masterApr 4, 2019
@wschin
wschin deleted the fix-value-map branch April 4, 2019 22:28
@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.

Reviewers

3 more reviewers
@Ivanidzo4kaIvanidzo4kaIvanidzo4ka left review comments
@sfilipisfilipisfilipi approved these changes
@singlissinglissinglis approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@wschinwschin

Labels

bugSomething isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

The ValueMap Train method assumes that keys are on the first column, and values on the second, rather than storing the columns and using those

4 participants

@wschin@Ivanidzo4ka@singlis@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
Fix a value-mapping bug - #3180
Merged
wschin merged 4 commits into
dotnet:masterfrom 
wschin:fix-value-map
Apr 4, 2019
Merged
Fix a value-mapping bug#3180
wschin merged 4 commits into
dotnet:masterfrom 
wschin:fix-value-map
Conversation
@wschin
@wschinwschin
commented
Apr 2, 2019

Copy link


Copy Markdown

Contributor
This PR fixes#3166.
@wschinwschin
added
the 
bug
Something isn't working
label
Apr 2, 2019
@wschinwschin
self-assigned this
Apr 2, 2019
@wschin
wschin
requested review from
sfilipi and singlisApril 2, 2019 21:43
@codecov
codecovBot
commented
Apr 2, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #3180 into master will increase coverage by 0.06%.

The diff coverage is 97.19%.
@@ Coverage Diff @@## master #3180 +/- ##
==========================================
+ Coverage 72.54% 72.6% +0.06% 
==========================================
Files 807 807 Lines 144774 145077 +303 Branches 16208 16213 +5 ==========================================
+ Hits 105021 105335 +314 + Misses 35339 35324 -15 - Partials 4414 4418 +4
FlagCoverage Δ
#Debug72.6% <97.19%> (+0.06%)⬆️
#production68.15% <91.42%> (+0.02%)⬆️
#test88.92% <100%> (+0.09%)⬆️
Impacted FilesCoverage Δ
...crosoft.ML.Tests/Transformers/ValueMappingTests.cs100% <100%> (ø)⬆️
src/Microsoft.ML.Data/Transforms/ValueMapping.cs84.26% <91.22%> (-0.14%)⬇️
...ML.Data/Transforms/ConversionsExtensionsCatalog.cs64.07% <92.3%> (+19.2%)⬆️
...c/Microsoft.ML.FastTree/Utils/ThreadTaskManager.cs79.48% <0%> (-20.52%)⬇️
src/Microsoft.ML.DataView/KeyDataViewType.cs74.57% <0%> (-3.76%)⬇️
src/Microsoft.ML.Maml/MAML.cs24.75% <0%> (-1.46%)⬇️
test/Microsoft.ML.Tests/ImagesTests.cs98.69% <0%> (-0.13%)⬇️
...Microsoft.ML.Tests/Transformers/NormalizerTests.cs100% <0%> (ø)⬆️
src/Microsoft.ML.Transforms/Text/TextCatalog.cs41.66% <0%> (ø)⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.26% <0%> (+0.15%)⬆️
... and 5 more
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
 /// <param name="keyColumn">Name of the key column in <paramref name="lookupMap"/>.</param>
 /// <param name="valueColumn">Name of the value column in <paramref name="lookupMap"/>.</param>
 /// <param name="columns">The list of names of the input columns to apply the transformation, and the name of the resulting column.</param>
 internal ValueMappingEstimator(IHostEnvironment env, IDataView lookupMap, DataViewSchema.Column keyColumn, DataViewSchema.Column valueColumn, params (string outputColumnName, string inputColumnName)[] columns)
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

If we are adding this constructor, is the one above (that takes a key column name and value column name) needed? #Resolved
@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.

+1, especially since the extension that calls into the ctor, takes in DataViewSchema.Column and passes down only the name of the column.
In reply to: 271533565 [](ancestors = 271533565)
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I will remove it in the next iteration.
In reply to: 271577343 [](ancestors = 271577343,271533565)
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 var valueType = dataView.Schema[valueIdx].Type;
 var valueMap = ValueMap.Create(keyType, valueType, ValueColumnMetadata);
 using (var cursor = dataView.GetRowCursor(dataView.Schema[keyIdx], dataView.Schema[valueIdx]))
 var retrievedKeyColumn = dataView.Schema[keyColumn.Index];
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

Schema [](start = 46, length = 6)
Can this be out of range? #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Will add a check in the next iteration.
In reply to: 271535033 [](ancestors = 271535033)
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 Host.Check(retrievedKeyColumn.Type == keyColumn.Type, nameof(keyColumn) + "'s column type doesn't match that of the associated column in " + nameof(dataView));
 Host.Check(retrievedKeyColumn.Annotations == keyColumn.Annotations, nameof(keyColumn) + "'s column annotions don't match those of the associated column in " + nameof(dataView));

 var retrievedValueColumn = dataView.Schema[valueColumn.Index];
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

same here. #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Will add a check as well.
In reply to: 271535097 [](ancestors = 271535097)
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 private readonly byte[] _dataView;

 private readonly DataViewSchema.Column _lookupKeyColumn;
 private readonly DataViewSchema.Column _lookupValueColumn;
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

private [](start = 8, length = 7)
Why store these? They look like they can be removed. #Resolved
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Super nice catch. Thanks.
In reply to: 271535842 [](ancestors = 271535842)
singlis
singlis
approved these changes
Apr 2, 2019
View reviewed changes
@singlissinglis
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:
Ivanidzo4ka
Ivanidzo4ka
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

@@ -46,7 +46,22 @@ public class ValueMappingEstimator : TrivialEstimator<ValueMappingTransformer>
 /// <param name="columns">The list of names of the input columns to apply the transformation, and the name of the resulting column.</param>
 internal ValueMappingEstimator(IHostEnvironment env, IDataView lookupMap, string keyColumn, string valueColumn, params (string outputColumnName, string inputColumnName)[] columns)
@Ivanidzo4kaIvanidzo4kaApr 3, 2019•

edited by sfilipi

Loading

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.

ValueMappingEstimator [](start = 17, length = 21)
I'm not getting why we still have this constructor

Public interface operates over Schema.Column object, any conversion from string to Schema.Column is unrelaable, why it's still around? #Closed
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Removed and related code is updated as well.
In reply to: 271544302 [](ancestors = 271544302)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
DataViewSchema.Column lookupKeyColumn, DataViewSchema.Column lookupValueColumn, (string outputColumnName, string inputColumnName)[] columns)
 : base(Contracts.CheckRef(env, nameof(env)).Register(nameof(ValueMappingTransformer)), columns)
 {
 Host.CheckNonEmpty(keyColumn, nameof(keyColumn), "A key column must be specified when passing in an IDataView for the value mapping");
@sfilipisfilipiApr 3, 2019•

edited

Loading

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.

Host.CheckNonEmpty [](start = 11, length = 19)
maybe check lookupKeyColumn.HasValue #Closed
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We can't. This is a struct, not a class.
In reply to: 271577590 [](ancestors = 271577590)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/Transformers/ValueMappingTests.cs
Outdated

 // Create a new ML context, for ML.NET operations. It can be used for exception tracking and logging, 
 // as well as the source of randomness.
 var mlContext = new MLContext();

@sfilipisfilipiApr 3, 2019•

edited

Loading

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.

you don't need this, the file should already have it: ML #Closed
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/Transformers/ValueMappingTests.cs
Outdated

 // Getting the resulting data as an IEnumerable.
 var features = mlContext.Data.CreateEnumerable<TransformedData>(transformedData, reuseRowObject: false).ToList();

 var expectedPrices = new float[] { 3.14f, 2000f, 1.19f, 2.17f, 33.784f };
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

expectedPrices [](start = 16, length = 14)
you have rawData storing those #Resolved
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yes. Thank you.
In reply to: 271578355 [](ancestors = 271578355)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
 _lookupValueColumn = lookupValueColumn;
 _valueMap = CreateValueMapFromDataView(lookupMap, _lookupKeyColumn, _lookupValueColumn);
 ValueColumnMetadata = lookupMap.Schema[_lookupValueColumn.Index].Annotations;

@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

Does _lookupValueColumn.Annotations work? #Resolved
@wschinwschinApr 4, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Just did a small refactorization around Annotations and extent a test to cover it at Iteration 4.
In reply to: 271578470 [](ancestors = 271578470)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 var retrievedKeyColumn = dataView.Schema[keyColumn.Index];
 Host.Check(retrievedKeyColumn.Index == keyColumn.Index, nameof(keyColumn) + "'s column index doesn't match that of the associated column in " + nameof(dataView));
 Host.Check(retrievedKeyColumn.Type == keyColumn.Type, nameof(keyColumn) + "'s column type doesn't match that of the associated column in " + nameof(dataView));
 Host.Check(retrievedKeyColumn.Annotations == keyColumn.Annotations, nameof(keyColumn) + "'s column annotions don't match those of the associated column in " + nameof(dataView));
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

annotions [](start = 111, length = 9)
typo #Resolved
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 var valueType = dataView.Schema[valueIdx].Type;
 var valueMap = ValueMap.Create(keyType, valueType, ValueColumnMetadata);
 using (var cursor = dataView.GetRowCursor(dataView.Schema[keyIdx], dataView.Schema[valueIdx]))
 var retrievedKeyColumn = dataView.Schema[keyColumn.Index];
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

retrievedKeyColumn [](start = 16, length = 18)
do we have any overload for Equals on the DataViewSchema.Column? The checks below seem like they would belong in that Equals. #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We don't.
In reply to: 271578862 [](ancestors = 271578862)
@sfilipisfilipiApr 4, 2019•

edited

Loading

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'll open a bug about it.. we should have it.
In reply to: 271966741 [](ancestors = 271966741,271578862)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 /// </summary>
 private abstract class ValueMap
 {
 public readonly string KeyColumnName;
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

KeyColumnName [](start = 35, length = 13)
what do you think about using the entire column, rather than the name? we have a mix of storing indices, names, columns.. #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I will merge Name and Type into DataViewSchema.Column and clean redundant functions up.
In reply to: 271579703 [](ancestors = 271579703)
sfilipi
sfilipi
approved these changes
Apr 4, 2019
View reviewed changes
@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:
sfilipi
sfilipi
reviewed
Apr 4, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ConversionsExtensionsCatalog.cs
 return new ValueMappingEstimator<TInputType, TOutputType>(catalog.GetEnvironment(), lookupMap,
 lookupMap.Schema[ValueMappingTransformer.DefaultKeyColumnName],
 lookupMap.Schema[ValueMappingTransformer.DefaultValueColumnName],
 InputOutputColumnPair.ConvertToValueTuples(columns));
@sfilipisfilipiApr 4, 2019•

edited

Loading

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.

there is a bit of repetition here, does it make sense to make it part of the ctor? I mean create the lookup map inside the ctor, just pass the columns.
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I feel columns must go with the associated IDataView. There should be no column if its IDataView hasn't been created.
@wschin
wschin
merged commit b8a70ac
into
dotnet:masterApr 4, 2019
@wschin
wschin
deleted the

fix-value-map

branch
April 4, 2019 22:28
@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.


Reviewers


3 more reviewers

@Ivanidzo4kaIvanidzo4kaIvanidzo4ka left review comments
@sfilipisfilipisfilipi approved these changes
@singlissinglissinglis approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@wschinwschin

Labels

bugSomething isn't working

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

The ValueMap Train method assumes that keys are on the first column, and values on the second, rather than storing the columns and using those


4 participants

@wschin@Ivanidzo4ka@singlis@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
Fix a value-mapping bug - #3180
Merged
wschin merged 4 commits into
dotnet:masterfrom 
wschin:fix-value-map
Apr 4, 2019
Merged
Fix a value-mapping bug#3180
wschin merged 4 commits into
dotnet:masterfrom 
wschin:fix-value-map
Conversation
@wschin
@wschinwschin
commented
Apr 2, 2019

Copy link


Copy Markdown

Contributor
This PR fixes#3166.
@wschinwschin
added
the 
bug
Something isn't working
label
Apr 2, 2019
@wschinwschin
self-assigned this
Apr 2, 2019
@wschin
wschin
requested review from
sfilipi and singlisApril 2, 2019 21:43
@codecov
codecovBot
commented
Apr 2, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #3180 into master will increase coverage by 0.06%.

The diff coverage is 97.19%.
@@ Coverage Diff @@## master #3180 +/- ##
==========================================
+ Coverage 72.54% 72.6% +0.06% 
==========================================
Files 807 807 Lines 144774 145077 +303 Branches 16208 16213 +5 ==========================================
+ Hits 105021 105335 +314 + Misses 35339 35324 -15 - Partials 4414 4418 +4
FlagCoverage Δ
#Debug72.6% <97.19%> (+0.06%)⬆️
#production68.15% <91.42%> (+0.02%)⬆️
#test88.92% <100%> (+0.09%)⬆️
Impacted FilesCoverage Δ
...crosoft.ML.Tests/Transformers/ValueMappingTests.cs100% <100%> (ø)⬆️
src/Microsoft.ML.Data/Transforms/ValueMapping.cs84.26% <91.22%> (-0.14%)⬇️
...ML.Data/Transforms/ConversionsExtensionsCatalog.cs64.07% <92.3%> (+19.2%)⬆️
...c/Microsoft.ML.FastTree/Utils/ThreadTaskManager.cs79.48% <0%> (-20.52%)⬇️
src/Microsoft.ML.DataView/KeyDataViewType.cs74.57% <0%> (-3.76%)⬇️
src/Microsoft.ML.Maml/MAML.cs24.75% <0%> (-1.46%)⬇️
test/Microsoft.ML.Tests/ImagesTests.cs98.69% <0%> (-0.13%)⬇️
...Microsoft.ML.Tests/Transformers/NormalizerTests.cs100% <0%> (ø)⬆️
src/Microsoft.ML.Transforms/Text/TextCatalog.cs41.66% <0%> (ø)⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.26% <0%> (+0.15%)⬆️
... and 5 more
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
 /// <param name="keyColumn">Name of the key column in <paramref name="lookupMap"/>.</param>
 /// <param name="valueColumn">Name of the value column in <paramref name="lookupMap"/>.</param>
 /// <param name="columns">The list of names of the input columns to apply the transformation, and the name of the resulting column.</param>
 internal ValueMappingEstimator(IHostEnvironment env, IDataView lookupMap, DataViewSchema.Column keyColumn, DataViewSchema.Column valueColumn, params (string outputColumnName, string inputColumnName)[] columns)
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

If we are adding this constructor, is the one above (that takes a key column name and value column name) needed? #Resolved
@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.

+1, especially since the extension that calls into the ctor, takes in DataViewSchema.Column and passes down only the name of the column.
In reply to: 271533565 [](ancestors = 271533565)
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I will remove it in the next iteration.
In reply to: 271577343 [](ancestors = 271577343,271533565)
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 var valueType = dataView.Schema[valueIdx].Type;
 var valueMap = ValueMap.Create(keyType, valueType, ValueColumnMetadata);
 using (var cursor = dataView.GetRowCursor(dataView.Schema[keyIdx], dataView.Schema[valueIdx]))
 var retrievedKeyColumn = dataView.Schema[keyColumn.Index];
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

Schema [](start = 46, length = 6)
Can this be out of range? #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Will add a check in the next iteration.
In reply to: 271535033 [](ancestors = 271535033)
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 Host.Check(retrievedKeyColumn.Type == keyColumn.Type, nameof(keyColumn) + "'s column type doesn't match that of the associated column in " + nameof(dataView));
 Host.Check(retrievedKeyColumn.Annotations == keyColumn.Annotations, nameof(keyColumn) + "'s column annotions don't match those of the associated column in " + nameof(dataView));

 var retrievedValueColumn = dataView.Schema[valueColumn.Index];
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

same here. #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Will add a check as well.
In reply to: 271535097 [](ancestors = 271535097)
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 private readonly byte[] _dataView;

 private readonly DataViewSchema.Column _lookupKeyColumn;
 private readonly DataViewSchema.Column _lookupValueColumn;
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

private [](start = 8, length = 7)
Why store these? They look like they can be removed. #Resolved
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Super nice catch. Thanks.
In reply to: 271535842 [](ancestors = 271535842)
singlis
singlis
approved these changes
Apr 2, 2019
View reviewed changes
@singlissinglis
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:
Ivanidzo4ka
Ivanidzo4ka
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

@@ -46,7 +46,22 @@ public class ValueMappingEstimator : TrivialEstimator<ValueMappingTransformer>
 /// <param name="columns">The list of names of the input columns to apply the transformation, and the name of the resulting column.</param>
 internal ValueMappingEstimator(IHostEnvironment env, IDataView lookupMap, string keyColumn, string valueColumn, params (string outputColumnName, string inputColumnName)[] columns)
@Ivanidzo4kaIvanidzo4kaApr 3, 2019•

edited by sfilipi

Loading

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.

ValueMappingEstimator [](start = 17, length = 21)
I'm not getting why we still have this constructor

Public interface operates over Schema.Column object, any conversion from string to Schema.Column is unrelaable, why it's still around? #Closed
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Removed and related code is updated as well.
In reply to: 271544302 [](ancestors = 271544302)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
DataViewSchema.Column lookupKeyColumn, DataViewSchema.Column lookupValueColumn, (string outputColumnName, string inputColumnName)[] columns)
 : base(Contracts.CheckRef(env, nameof(env)).Register(nameof(ValueMappingTransformer)), columns)
 {
 Host.CheckNonEmpty(keyColumn, nameof(keyColumn), "A key column must be specified when passing in an IDataView for the value mapping");
@sfilipisfilipiApr 3, 2019•

edited

Loading

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.

Host.CheckNonEmpty [](start = 11, length = 19)
maybe check lookupKeyColumn.HasValue #Closed
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We can't. This is a struct, not a class.
In reply to: 271577590 [](ancestors = 271577590)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/Transformers/ValueMappingTests.cs
Outdated

 // Create a new ML context, for ML.NET operations. It can be used for exception tracking and logging, 
 // as well as the source of randomness.
 var mlContext = new MLContext();

@sfilipisfilipiApr 3, 2019•

edited

Loading

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.

you don't need this, the file should already have it: ML #Closed
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/Transformers/ValueMappingTests.cs
Outdated

 // Getting the resulting data as an IEnumerable.
 var features = mlContext.Data.CreateEnumerable<TransformedData>(transformedData, reuseRowObject: false).ToList();

 var expectedPrices = new float[] { 3.14f, 2000f, 1.19f, 2.17f, 33.784f };
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

expectedPrices [](start = 16, length = 14)
you have rawData storing those #Resolved
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yes. Thank you.
In reply to: 271578355 [](ancestors = 271578355)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
 _lookupValueColumn = lookupValueColumn;
 _valueMap = CreateValueMapFromDataView(lookupMap, _lookupKeyColumn, _lookupValueColumn);
 ValueColumnMetadata = lookupMap.Schema[_lookupValueColumn.Index].Annotations;

@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

Does _lookupValueColumn.Annotations work? #Resolved
@wschinwschinApr 4, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Just did a small refactorization around Annotations and extent a test to cover it at Iteration 4.
In reply to: 271578470 [](ancestors = 271578470)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 var retrievedKeyColumn = dataView.Schema[keyColumn.Index];
 Host.Check(retrievedKeyColumn.Index == keyColumn.Index, nameof(keyColumn) + "'s column index doesn't match that of the associated column in " + nameof(dataView));
 Host.Check(retrievedKeyColumn.Type == keyColumn.Type, nameof(keyColumn) + "'s column type doesn't match that of the associated column in " + nameof(dataView));
 Host.Check(retrievedKeyColumn.Annotations == keyColumn.Annotations, nameof(keyColumn) + "'s column annotions don't match those of the associated column in " + nameof(dataView));
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

annotions [](start = 111, length = 9)
typo #Resolved
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 var valueType = dataView.Schema[valueIdx].Type;
 var valueMap = ValueMap.Create(keyType, valueType, ValueColumnMetadata);
 using (var cursor = dataView.GetRowCursor(dataView.Schema[keyIdx], dataView.Schema[valueIdx]))
 var retrievedKeyColumn = dataView.Schema[keyColumn.Index];
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

retrievedKeyColumn [](start = 16, length = 18)
do we have any overload for Equals on the DataViewSchema.Column? The checks below seem like they would belong in that Equals. #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We don't.
In reply to: 271578862 [](ancestors = 271578862)
@sfilipisfilipiApr 4, 2019•

edited

Loading

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'll open a bug about it.. we should have it.
In reply to: 271966741 [](ancestors = 271966741,271578862)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 /// </summary>
 private abstract class ValueMap
 {
 public readonly string KeyColumnName;
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

KeyColumnName [](start = 35, length = 13)
what do you think about using the entire column, rather than the name? we have a mix of storing indices, names, columns.. #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I will merge Name and Type into DataViewSchema.Column and clean redundant functions up.
In reply to: 271579703 [](ancestors = 271579703)
sfilipi
sfilipi
approved these changes
Apr 4, 2019
View reviewed changes
@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:
sfilipi
sfilipi
reviewed
Apr 4, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ConversionsExtensionsCatalog.cs
 return new ValueMappingEstimator<TInputType, TOutputType>(catalog.GetEnvironment(), lookupMap,
 lookupMap.Schema[ValueMappingTransformer.DefaultKeyColumnName],
 lookupMap.Schema[ValueMappingTransformer.DefaultValueColumnName],
 InputOutputColumnPair.ConvertToValueTuples(columns));
@sfilipisfilipiApr 4, 2019•

edited

Loading

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.

there is a bit of repetition here, does it make sense to make it part of the ctor? I mean create the lookup map inside the ctor, just pass the columns.
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I feel columns must go with the associated IDataView. There should be no column if its IDataView hasn't been created.
@wschin
wschin
merged commit b8a70ac
into
dotnet:masterApr 4, 2019
@wschin
wschin
deleted the

fix-value-map

branch
April 4, 2019 22:28
@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.


Reviewers


3 more reviewers

@Ivanidzo4kaIvanidzo4kaIvanidzo4ka left review comments
@sfilipisfilipisfilipi approved these changes
@singlissinglissinglis approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@wschinwschin

Labels

bugSomething isn't working

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

The ValueMap Train method assumes that keys are on the first column, and values on the second, rather than storing the columns and using those


4 participants

@wschin@Ivanidzo4ka@singlis@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
Fix a value-mapping bug - #3180
Merged
wschin merged 4 commits into
dotnet:masterfrom 
wschin:fix-value-map
Apr 4, 2019
Merged
Fix a value-mapping bug#3180
wschin merged 4 commits into
dotnet:masterfrom 
wschin:fix-value-map
Conversation
@wschin
@wschinwschin
commented
Apr 2, 2019

Copy link


Copy Markdown

Contributor
This PR fixes#3166.
@wschinwschin
added
the 
bug
Something isn't working
label
Apr 2, 2019
@wschinwschin
self-assigned this
Apr 2, 2019
@wschin
wschin
requested review from
sfilipi and singlisApril 2, 2019 21:43
@codecov
codecovBot
commented
Apr 2, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #3180 into master will increase coverage by 0.06%.

The diff coverage is 97.19%.
@@ Coverage Diff @@## master #3180 +/- ##
==========================================
+ Coverage 72.54% 72.6% +0.06% 
==========================================
Files 807 807 Lines 144774 145077 +303 Branches 16208 16213 +5 ==========================================
+ Hits 105021 105335 +314 + Misses 35339 35324 -15 - Partials 4414 4418 +4
FlagCoverage Δ
#Debug72.6% <97.19%> (+0.06%)⬆️
#production68.15% <91.42%> (+0.02%)⬆️
#test88.92% <100%> (+0.09%)⬆️
Impacted FilesCoverage Δ
...crosoft.ML.Tests/Transformers/ValueMappingTests.cs100% <100%> (ø)⬆️
src/Microsoft.ML.Data/Transforms/ValueMapping.cs84.26% <91.22%> (-0.14%)⬇️
...ML.Data/Transforms/ConversionsExtensionsCatalog.cs64.07% <92.3%> (+19.2%)⬆️
...c/Microsoft.ML.FastTree/Utils/ThreadTaskManager.cs79.48% <0%> (-20.52%)⬇️
src/Microsoft.ML.DataView/KeyDataViewType.cs74.57% <0%> (-3.76%)⬇️
src/Microsoft.ML.Maml/MAML.cs24.75% <0%> (-1.46%)⬇️
test/Microsoft.ML.Tests/ImagesTests.cs98.69% <0%> (-0.13%)⬇️
...Microsoft.ML.Tests/Transformers/NormalizerTests.cs100% <0%> (ø)⬆️
src/Microsoft.ML.Transforms/Text/TextCatalog.cs41.66% <0%> (ø)⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.26% <0%> (+0.15%)⬆️
... and 5 more
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
 /// <param name="keyColumn">Name of the key column in <paramref name="lookupMap"/>.</param>
 /// <param name="valueColumn">Name of the value column in <paramref name="lookupMap"/>.</param>
 /// <param name="columns">The list of names of the input columns to apply the transformation, and the name of the resulting column.</param>
 internal ValueMappingEstimator(IHostEnvironment env, IDataView lookupMap, DataViewSchema.Column keyColumn, DataViewSchema.Column valueColumn, params (string outputColumnName, string inputColumnName)[] columns)
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

If we are adding this constructor, is the one above (that takes a key column name and value column name) needed? #Resolved
@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.

+1, especially since the extension that calls into the ctor, takes in DataViewSchema.Column and passes down only the name of the column.
In reply to: 271533565 [](ancestors = 271533565)
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I will remove it in the next iteration.
In reply to: 271577343 [](ancestors = 271577343,271533565)
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 var valueType = dataView.Schema[valueIdx].Type;
 var valueMap = ValueMap.Create(keyType, valueType, ValueColumnMetadata);
 using (var cursor = dataView.GetRowCursor(dataView.Schema[keyIdx], dataView.Schema[valueIdx]))
 var retrievedKeyColumn = dataView.Schema[keyColumn.Index];
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

Schema [](start = 46, length = 6)
Can this be out of range? #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Will add a check in the next iteration.
In reply to: 271535033 [](ancestors = 271535033)
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 Host.Check(retrievedKeyColumn.Type == keyColumn.Type, nameof(keyColumn) + "'s column type doesn't match that of the associated column in " + nameof(dataView));
 Host.Check(retrievedKeyColumn.Annotations == keyColumn.Annotations, nameof(keyColumn) + "'s column annotions don't match those of the associated column in " + nameof(dataView));

 var retrievedValueColumn = dataView.Schema[valueColumn.Index];
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

same here. #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Will add a check as well.
In reply to: 271535097 [](ancestors = 271535097)
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 private readonly byte[] _dataView;

 private readonly DataViewSchema.Column _lookupKeyColumn;
 private readonly DataViewSchema.Column _lookupValueColumn;
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

private [](start = 8, length = 7)
Why store these? They look like they can be removed. #Resolved
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Super nice catch. Thanks.
In reply to: 271535842 [](ancestors = 271535842)
singlis
singlis
approved these changes
Apr 2, 2019
View reviewed changes
@singlissinglis
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:
Ivanidzo4ka
Ivanidzo4ka
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

@@ -46,7 +46,22 @@ public class ValueMappingEstimator : TrivialEstimator<ValueMappingTransformer>
 /// <param name="columns">The list of names of the input columns to apply the transformation, and the name of the resulting column.</param>
 internal ValueMappingEstimator(IHostEnvironment env, IDataView lookupMap, string keyColumn, string valueColumn, params (string outputColumnName, string inputColumnName)[] columns)
@Ivanidzo4kaIvanidzo4kaApr 3, 2019•

edited by sfilipi

Loading

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.

ValueMappingEstimator [](start = 17, length = 21)
I'm not getting why we still have this constructor

Public interface operates over Schema.Column object, any conversion from string to Schema.Column is unrelaable, why it's still around? #Closed
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Removed and related code is updated as well.
In reply to: 271544302 [](ancestors = 271544302)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
DataViewSchema.Column lookupKeyColumn, DataViewSchema.Column lookupValueColumn, (string outputColumnName, string inputColumnName)[] columns)
 : base(Contracts.CheckRef(env, nameof(env)).Register(nameof(ValueMappingTransformer)), columns)
 {
 Host.CheckNonEmpty(keyColumn, nameof(keyColumn), "A key column must be specified when passing in an IDataView for the value mapping");
@sfilipisfilipiApr 3, 2019•

edited

Loading

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.

Host.CheckNonEmpty [](start = 11, length = 19)
maybe check lookupKeyColumn.HasValue #Closed
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We can't. This is a struct, not a class.
In reply to: 271577590 [](ancestors = 271577590)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/Transformers/ValueMappingTests.cs
Outdated

 // Create a new ML context, for ML.NET operations. It can be used for exception tracking and logging, 
 // as well as the source of randomness.
 var mlContext = new MLContext();

@sfilipisfilipiApr 3, 2019•

edited

Loading

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.

you don't need this, the file should already have it: ML #Closed
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/Transformers/ValueMappingTests.cs
Outdated

 // Getting the resulting data as an IEnumerable.
 var features = mlContext.Data.CreateEnumerable<TransformedData>(transformedData, reuseRowObject: false).ToList();

 var expectedPrices = new float[] { 3.14f, 2000f, 1.19f, 2.17f, 33.784f };
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

expectedPrices [](start = 16, length = 14)
you have rawData storing those #Resolved
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yes. Thank you.
In reply to: 271578355 [](ancestors = 271578355)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
 _lookupValueColumn = lookupValueColumn;
 _valueMap = CreateValueMapFromDataView(lookupMap, _lookupKeyColumn, _lookupValueColumn);
 ValueColumnMetadata = lookupMap.Schema[_lookupValueColumn.Index].Annotations;

@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

Does _lookupValueColumn.Annotations work? #Resolved
@wschinwschinApr 4, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Just did a small refactorization around Annotations and extent a test to cover it at Iteration 4.
In reply to: 271578470 [](ancestors = 271578470)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 var retrievedKeyColumn = dataView.Schema[keyColumn.Index];
 Host.Check(retrievedKeyColumn.Index == keyColumn.Index, nameof(keyColumn) + "'s column index doesn't match that of the associated column in " + nameof(dataView));
 Host.Check(retrievedKeyColumn.Type == keyColumn.Type, nameof(keyColumn) + "'s column type doesn't match that of the associated column in " + nameof(dataView));
 Host.Check(retrievedKeyColumn.Annotations == keyColumn.Annotations, nameof(keyColumn) + "'s column annotions don't match those of the associated column in " + nameof(dataView));
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

annotions [](start = 111, length = 9)
typo #Resolved
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 var valueType = dataView.Schema[valueIdx].Type;
 var valueMap = ValueMap.Create(keyType, valueType, ValueColumnMetadata);
 using (var cursor = dataView.GetRowCursor(dataView.Schema[keyIdx], dataView.Schema[valueIdx]))
 var retrievedKeyColumn = dataView.Schema[keyColumn.Index];
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

retrievedKeyColumn [](start = 16, length = 18)
do we have any overload for Equals on the DataViewSchema.Column? The checks below seem like they would belong in that Equals. #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We don't.
In reply to: 271578862 [](ancestors = 271578862)
@sfilipisfilipiApr 4, 2019•

edited

Loading

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'll open a bug about it.. we should have it.
In reply to: 271966741 [](ancestors = 271966741,271578862)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 /// </summary>
 private abstract class ValueMap
 {
 public readonly string KeyColumnName;
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

KeyColumnName [](start = 35, length = 13)
what do you think about using the entire column, rather than the name? we have a mix of storing indices, names, columns.. #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I will merge Name and Type into DataViewSchema.Column and clean redundant functions up.
In reply to: 271579703 [](ancestors = 271579703)
sfilipi
sfilipi
approved these changes
Apr 4, 2019
View reviewed changes
@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:
sfilipi
sfilipi
reviewed
Apr 4, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ConversionsExtensionsCatalog.cs
 return new ValueMappingEstimator<TInputType, TOutputType>(catalog.GetEnvironment(), lookupMap,
 lookupMap.Schema[ValueMappingTransformer.DefaultKeyColumnName],
 lookupMap.Schema[ValueMappingTransformer.DefaultValueColumnName],
 InputOutputColumnPair.ConvertToValueTuples(columns));
@sfilipisfilipiApr 4, 2019•

edited

Loading

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.

there is a bit of repetition here, does it make sense to make it part of the ctor? I mean create the lookup map inside the ctor, just pass the columns.
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I feel columns must go with the associated IDataView. There should be no column if its IDataView hasn't been created.
@wschin
wschin
merged commit b8a70ac
into
dotnet:masterApr 4, 2019
@wschin
wschin
deleted the

fix-value-map

branch
April 4, 2019 22:28
@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.


Reviewers


3 more reviewers

@Ivanidzo4kaIvanidzo4kaIvanidzo4ka left review comments
@sfilipisfilipisfilipi approved these changes
@singlissinglissinglis approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@wschinwschin

Labels

bugSomething isn't working

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

The ValueMap Train method assumes that keys are on the first column, and values on the second, rather than storing the columns and using those


4 participants

@wschin@Ivanidzo4ka@singlis@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
Fix a value-mapping bug - #3180
Merged
wschin merged 4 commits into
dotnet:masterfrom 
wschin:fix-value-map
Apr 4, 2019
Merged
Fix a value-mapping bug#3180
wschin merged 4 commits into
dotnet:masterfrom 
wschin:fix-value-map
Conversation
@wschin
@wschinwschin
commented
Apr 2, 2019

Copy link


Copy Markdown

Contributor
This PR fixes#3166.
@wschinwschin
added
the 
bug
Something isn't working
label
Apr 2, 2019
@wschinwschin
self-assigned this
Apr 2, 2019
@wschin
wschin
requested review from
sfilipi and singlisApril 2, 2019 21:43
@codecov
codecovBot
commented
Apr 2, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #3180 into master will increase coverage by 0.06%.

The diff coverage is 97.19%.
@@ Coverage Diff @@## master #3180 +/- ##
==========================================
+ Coverage 72.54% 72.6% +0.06% 
==========================================
Files 807 807 Lines 144774 145077 +303 Branches 16208 16213 +5 ==========================================
+ Hits 105021 105335 +314 + Misses 35339 35324 -15 - Partials 4414 4418 +4
FlagCoverage Δ
#Debug72.6% <97.19%> (+0.06%)⬆️
#production68.15% <91.42%> (+0.02%)⬆️
#test88.92% <100%> (+0.09%)⬆️
Impacted FilesCoverage Δ
...crosoft.ML.Tests/Transformers/ValueMappingTests.cs100% <100%> (ø)⬆️
src/Microsoft.ML.Data/Transforms/ValueMapping.cs84.26% <91.22%> (-0.14%)⬇️
...ML.Data/Transforms/ConversionsExtensionsCatalog.cs64.07% <92.3%> (+19.2%)⬆️
...c/Microsoft.ML.FastTree/Utils/ThreadTaskManager.cs79.48% <0%> (-20.52%)⬇️
src/Microsoft.ML.DataView/KeyDataViewType.cs74.57% <0%> (-3.76%)⬇️
src/Microsoft.ML.Maml/MAML.cs24.75% <0%> (-1.46%)⬇️
test/Microsoft.ML.Tests/ImagesTests.cs98.69% <0%> (-0.13%)⬇️
...Microsoft.ML.Tests/Transformers/NormalizerTests.cs100% <0%> (ø)⬆️
src/Microsoft.ML.Transforms/Text/TextCatalog.cs41.66% <0%> (ø)⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.26% <0%> (+0.15%)⬆️
... and 5 more
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
 /// <param name="keyColumn">Name of the key column in <paramref name="lookupMap"/>.</param>
 /// <param name="valueColumn">Name of the value column in <paramref name="lookupMap"/>.</param>
 /// <param name="columns">The list of names of the input columns to apply the transformation, and the name of the resulting column.</param>
 internal ValueMappingEstimator(IHostEnvironment env, IDataView lookupMap, DataViewSchema.Column keyColumn, DataViewSchema.Column valueColumn, params (string outputColumnName, string inputColumnName)[] columns)
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

If we are adding this constructor, is the one above (that takes a key column name and value column name) needed? #Resolved
@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.

+1, especially since the extension that calls into the ctor, takes in DataViewSchema.Column and passes down only the name of the column.
In reply to: 271533565 [](ancestors = 271533565)
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I will remove it in the next iteration.
In reply to: 271577343 [](ancestors = 271577343,271533565)
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 var valueType = dataView.Schema[valueIdx].Type;
 var valueMap = ValueMap.Create(keyType, valueType, ValueColumnMetadata);
 using (var cursor = dataView.GetRowCursor(dataView.Schema[keyIdx], dataView.Schema[valueIdx]))
 var retrievedKeyColumn = dataView.Schema[keyColumn.Index];
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

Schema [](start = 46, length = 6)
Can this be out of range? #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Will add a check in the next iteration.
In reply to: 271535033 [](ancestors = 271535033)
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 Host.Check(retrievedKeyColumn.Type == keyColumn.Type, nameof(keyColumn) + "'s column type doesn't match that of the associated column in " + nameof(dataView));
 Host.Check(retrievedKeyColumn.Annotations == keyColumn.Annotations, nameof(keyColumn) + "'s column annotions don't match those of the associated column in " + nameof(dataView));

 var retrievedValueColumn = dataView.Schema[valueColumn.Index];
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

same here. #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Will add a check as well.
In reply to: 271535097 [](ancestors = 271535097)
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 private readonly byte[] _dataView;

 private readonly DataViewSchema.Column _lookupKeyColumn;
 private readonly DataViewSchema.Column _lookupValueColumn;
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

private [](start = 8, length = 7)
Why store these? They look like they can be removed. #Resolved
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Super nice catch. Thanks.
In reply to: 271535842 [](ancestors = 271535842)
singlis
singlis
approved these changes
Apr 2, 2019
View reviewed changes
@singlissinglis
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:
Ivanidzo4ka
Ivanidzo4ka
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

@@ -46,7 +46,22 @@ public class ValueMappingEstimator : TrivialEstimator<ValueMappingTransformer>
 /// <param name="columns">The list of names of the input columns to apply the transformation, and the name of the resulting column.</param>
 internal ValueMappingEstimator(IHostEnvironment env, IDataView lookupMap, string keyColumn, string valueColumn, params (string outputColumnName, string inputColumnName)[] columns)
@Ivanidzo4kaIvanidzo4kaApr 3, 2019•

edited by sfilipi

Loading

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.

ValueMappingEstimator [](start = 17, length = 21)
I'm not getting why we still have this constructor

Public interface operates over Schema.Column object, any conversion from string to Schema.Column is unrelaable, why it's still around? #Closed
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Removed and related code is updated as well.
In reply to: 271544302 [](ancestors = 271544302)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
DataViewSchema.Column lookupKeyColumn, DataViewSchema.Column lookupValueColumn, (string outputColumnName, string inputColumnName)[] columns)
 : base(Contracts.CheckRef(env, nameof(env)).Register(nameof(ValueMappingTransformer)), columns)
 {
 Host.CheckNonEmpty(keyColumn, nameof(keyColumn), "A key column must be specified when passing in an IDataView for the value mapping");
@sfilipisfilipiApr 3, 2019•

edited

Loading

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.

Host.CheckNonEmpty [](start = 11, length = 19)
maybe check lookupKeyColumn.HasValue #Closed
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We can't. This is a struct, not a class.
In reply to: 271577590 [](ancestors = 271577590)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/Transformers/ValueMappingTests.cs
Outdated

 // Create a new ML context, for ML.NET operations. It can be used for exception tracking and logging, 
 // as well as the source of randomness.
 var mlContext = new MLContext();

@sfilipisfilipiApr 3, 2019•

edited

Loading

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.

you don't need this, the file should already have it: ML #Closed
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/Transformers/ValueMappingTests.cs
Outdated

 // Getting the resulting data as an IEnumerable.
 var features = mlContext.Data.CreateEnumerable<TransformedData>(transformedData, reuseRowObject: false).ToList();

 var expectedPrices = new float[] { 3.14f, 2000f, 1.19f, 2.17f, 33.784f };
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

expectedPrices [](start = 16, length = 14)
you have rawData storing those #Resolved
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yes. Thank you.
In reply to: 271578355 [](ancestors = 271578355)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
 _lookupValueColumn = lookupValueColumn;
 _valueMap = CreateValueMapFromDataView(lookupMap, _lookupKeyColumn, _lookupValueColumn);
 ValueColumnMetadata = lookupMap.Schema[_lookupValueColumn.Index].Annotations;

@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

Does _lookupValueColumn.Annotations work? #Resolved
@wschinwschinApr 4, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Just did a small refactorization around Annotations and extent a test to cover it at Iteration 4.
In reply to: 271578470 [](ancestors = 271578470)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 var retrievedKeyColumn = dataView.Schema[keyColumn.Index];
 Host.Check(retrievedKeyColumn.Index == keyColumn.Index, nameof(keyColumn) + "'s column index doesn't match that of the associated column in " + nameof(dataView));
 Host.Check(retrievedKeyColumn.Type == keyColumn.Type, nameof(keyColumn) + "'s column type doesn't match that of the associated column in " + nameof(dataView));
 Host.Check(retrievedKeyColumn.Annotations == keyColumn.Annotations, nameof(keyColumn) + "'s column annotions don't match those of the associated column in " + nameof(dataView));
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

annotions [](start = 111, length = 9)
typo #Resolved
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 var valueType = dataView.Schema[valueIdx].Type;
 var valueMap = ValueMap.Create(keyType, valueType, ValueColumnMetadata);
 using (var cursor = dataView.GetRowCursor(dataView.Schema[keyIdx], dataView.Schema[valueIdx]))
 var retrievedKeyColumn = dataView.Schema[keyColumn.Index];
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

retrievedKeyColumn [](start = 16, length = 18)
do we have any overload for Equals on the DataViewSchema.Column? The checks below seem like they would belong in that Equals. #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We don't.
In reply to: 271578862 [](ancestors = 271578862)
@sfilipisfilipiApr 4, 2019•

edited

Loading

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'll open a bug about it.. we should have it.
In reply to: 271966741 [](ancestors = 271966741,271578862)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 /// </summary>
 private abstract class ValueMap
 {
 public readonly string KeyColumnName;
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

KeyColumnName [](start = 35, length = 13)
what do you think about using the entire column, rather than the name? we have a mix of storing indices, names, columns.. #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I will merge Name and Type into DataViewSchema.Column and clean redundant functions up.
In reply to: 271579703 [](ancestors = 271579703)
sfilipi
sfilipi
approved these changes
Apr 4, 2019
View reviewed changes
@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:
sfilipi
sfilipi
reviewed
Apr 4, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ConversionsExtensionsCatalog.cs
 return new ValueMappingEstimator<TInputType, TOutputType>(catalog.GetEnvironment(), lookupMap,
 lookupMap.Schema[ValueMappingTransformer.DefaultKeyColumnName],
 lookupMap.Schema[ValueMappingTransformer.DefaultValueColumnName],
 InputOutputColumnPair.ConvertToValueTuples(columns));
@sfilipisfilipiApr 4, 2019•

edited

Loading

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.

there is a bit of repetition here, does it make sense to make it part of the ctor? I mean create the lookup map inside the ctor, just pass the columns.
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I feel columns must go with the associated IDataView. There should be no column if its IDataView hasn't been created.
@wschin
wschin
merged commit b8a70ac
into
dotnet:masterApr 4, 2019
@wschin
wschin
deleted the

fix-value-map

branch
April 4, 2019 22:28
@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.


Reviewers


3 more reviewers

@Ivanidzo4kaIvanidzo4kaIvanidzo4ka left review comments
@sfilipisfilipisfilipi approved these changes
@singlissinglissinglis approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@wschinwschin

Labels

bugSomething isn't working

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

The ValueMap Train method assumes that keys are on the first column, and values on the second, rather than storing the columns and using those


4 participants

@wschin@Ivanidzo4ka@singlis@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
Fix a value-mapping bug - #3180
Merged
wschin merged 4 commits into
dotnet:masterfrom 
wschin:fix-value-map
Apr 4, 2019
Merged
Fix a value-mapping bug#3180
wschin merged 4 commits into
dotnet:masterfrom 
wschin:fix-value-map
Conversation
@wschin
@wschinwschin
commented
Apr 2, 2019

Copy link


Copy Markdown

Contributor
This PR fixes#3166.
@wschinwschin
added
the 
bug
Something isn't working
label
Apr 2, 2019
@wschinwschin
self-assigned this
Apr 2, 2019
@wschin
wschin
requested review from
sfilipi and singlisApril 2, 2019 21:43
@codecov
codecovBot
commented
Apr 2, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #3180 into master will increase coverage by 0.06%.

The diff coverage is 97.19%.
@@ Coverage Diff @@## master #3180 +/- ##
==========================================
+ Coverage 72.54% 72.6% +0.06% 
==========================================
Files 807 807 Lines 144774 145077 +303 Branches 16208 16213 +5 ==========================================
+ Hits 105021 105335 +314 + Misses 35339 35324 -15 - Partials 4414 4418 +4
FlagCoverage Δ
#Debug72.6% <97.19%> (+0.06%)⬆️
#production68.15% <91.42%> (+0.02%)⬆️
#test88.92% <100%> (+0.09%)⬆️
Impacted FilesCoverage Δ
...crosoft.ML.Tests/Transformers/ValueMappingTests.cs100% <100%> (ø)⬆️
src/Microsoft.ML.Data/Transforms/ValueMapping.cs84.26% <91.22%> (-0.14%)⬇️
...ML.Data/Transforms/ConversionsExtensionsCatalog.cs64.07% <92.3%> (+19.2%)⬆️
...c/Microsoft.ML.FastTree/Utils/ThreadTaskManager.cs79.48% <0%> (-20.52%)⬇️
src/Microsoft.ML.DataView/KeyDataViewType.cs74.57% <0%> (-3.76%)⬇️
src/Microsoft.ML.Maml/MAML.cs24.75% <0%> (-1.46%)⬇️
test/Microsoft.ML.Tests/ImagesTests.cs98.69% <0%> (-0.13%)⬇️
...Microsoft.ML.Tests/Transformers/NormalizerTests.cs100% <0%> (ø)⬆️
src/Microsoft.ML.Transforms/Text/TextCatalog.cs41.66% <0%> (ø)⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.26% <0%> (+0.15%)⬆️
... and 5 more
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
 /// <param name="keyColumn">Name of the key column in <paramref name="lookupMap"/>.</param>
 /// <param name="valueColumn">Name of the value column in <paramref name="lookupMap"/>.</param>
 /// <param name="columns">The list of names of the input columns to apply the transformation, and the name of the resulting column.</param>
 internal ValueMappingEstimator(IHostEnvironment env, IDataView lookupMap, DataViewSchema.Column keyColumn, DataViewSchema.Column valueColumn, params (string outputColumnName, string inputColumnName)[] columns)
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

If we are adding this constructor, is the one above (that takes a key column name and value column name) needed? #Resolved
@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.

+1, especially since the extension that calls into the ctor, takes in DataViewSchema.Column and passes down only the name of the column.
In reply to: 271533565 [](ancestors = 271533565)
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I will remove it in the next iteration.
In reply to: 271577343 [](ancestors = 271577343,271533565)
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 var valueType = dataView.Schema[valueIdx].Type;
 var valueMap = ValueMap.Create(keyType, valueType, ValueColumnMetadata);
 using (var cursor = dataView.GetRowCursor(dataView.Schema[keyIdx], dataView.Schema[valueIdx]))
 var retrievedKeyColumn = dataView.Schema[keyColumn.Index];
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

Schema [](start = 46, length = 6)
Can this be out of range? #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Will add a check in the next iteration.
In reply to: 271535033 [](ancestors = 271535033)
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 Host.Check(retrievedKeyColumn.Type == keyColumn.Type, nameof(keyColumn) + "'s column type doesn't match that of the associated column in " + nameof(dataView));
 Host.Check(retrievedKeyColumn.Annotations == keyColumn.Annotations, nameof(keyColumn) + "'s column annotions don't match those of the associated column in " + nameof(dataView));

 var retrievedValueColumn = dataView.Schema[valueColumn.Index];
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

same here. #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Will add a check as well.
In reply to: 271535097 [](ancestors = 271535097)
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 private readonly byte[] _dataView;

 private readonly DataViewSchema.Column _lookupKeyColumn;
 private readonly DataViewSchema.Column _lookupValueColumn;
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

private [](start = 8, length = 7)
Why store these? They look like they can be removed. #Resolved
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Super nice catch. Thanks.
In reply to: 271535842 [](ancestors = 271535842)
singlis
singlis
approved these changes
Apr 2, 2019
View reviewed changes
@singlissinglis
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:
Ivanidzo4ka
Ivanidzo4ka
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

@@ -46,7 +46,22 @@ public class ValueMappingEstimator : TrivialEstimator<ValueMappingTransformer>
 /// <param name="columns">The list of names of the input columns to apply the transformation, and the name of the resulting column.</param>
 internal ValueMappingEstimator(IHostEnvironment env, IDataView lookupMap, string keyColumn, string valueColumn, params (string outputColumnName, string inputColumnName)[] columns)
@Ivanidzo4kaIvanidzo4kaApr 3, 2019•

edited by sfilipi

Loading

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.

ValueMappingEstimator [](start = 17, length = 21)
I'm not getting why we still have this constructor

Public interface operates over Schema.Column object, any conversion from string to Schema.Column is unrelaable, why it's still around? #Closed
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Removed and related code is updated as well.
In reply to: 271544302 [](ancestors = 271544302)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
DataViewSchema.Column lookupKeyColumn, DataViewSchema.Column lookupValueColumn, (string outputColumnName, string inputColumnName)[] columns)
 : base(Contracts.CheckRef(env, nameof(env)).Register(nameof(ValueMappingTransformer)), columns)
 {
 Host.CheckNonEmpty(keyColumn, nameof(keyColumn), "A key column must be specified when passing in an IDataView for the value mapping");
@sfilipisfilipiApr 3, 2019•

edited

Loading

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.

Host.CheckNonEmpty [](start = 11, length = 19)
maybe check lookupKeyColumn.HasValue #Closed
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We can't. This is a struct, not a class.
In reply to: 271577590 [](ancestors = 271577590)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/Transformers/ValueMappingTests.cs
Outdated

 // Create a new ML context, for ML.NET operations. It can be used for exception tracking and logging, 
 // as well as the source of randomness.
 var mlContext = new MLContext();

@sfilipisfilipiApr 3, 2019•

edited

Loading

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.

you don't need this, the file should already have it: ML #Closed
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/Transformers/ValueMappingTests.cs
Outdated

 // Getting the resulting data as an IEnumerable.
 var features = mlContext.Data.CreateEnumerable<TransformedData>(transformedData, reuseRowObject: false).ToList();

 var expectedPrices = new float[] { 3.14f, 2000f, 1.19f, 2.17f, 33.784f };
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

expectedPrices [](start = 16, length = 14)
you have rawData storing those #Resolved
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yes. Thank you.
In reply to: 271578355 [](ancestors = 271578355)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
 _lookupValueColumn = lookupValueColumn;
 _valueMap = CreateValueMapFromDataView(lookupMap, _lookupKeyColumn, _lookupValueColumn);
 ValueColumnMetadata = lookupMap.Schema[_lookupValueColumn.Index].Annotations;

@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

Does _lookupValueColumn.Annotations work? #Resolved
@wschinwschinApr 4, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Just did a small refactorization around Annotations and extent a test to cover it at Iteration 4.
In reply to: 271578470 [](ancestors = 271578470)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 var retrievedKeyColumn = dataView.Schema[keyColumn.Index];
 Host.Check(retrievedKeyColumn.Index == keyColumn.Index, nameof(keyColumn) + "'s column index doesn't match that of the associated column in " + nameof(dataView));
 Host.Check(retrievedKeyColumn.Type == keyColumn.Type, nameof(keyColumn) + "'s column type doesn't match that of the associated column in " + nameof(dataView));
 Host.Check(retrievedKeyColumn.Annotations == keyColumn.Annotations, nameof(keyColumn) + "'s column annotions don't match those of the associated column in " + nameof(dataView));
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

annotions [](start = 111, length = 9)
typo #Resolved
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 var valueType = dataView.Schema[valueIdx].Type;
 var valueMap = ValueMap.Create(keyType, valueType, ValueColumnMetadata);
 using (var cursor = dataView.GetRowCursor(dataView.Schema[keyIdx], dataView.Schema[valueIdx]))
 var retrievedKeyColumn = dataView.Schema[keyColumn.Index];
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

retrievedKeyColumn [](start = 16, length = 18)
do we have any overload for Equals on the DataViewSchema.Column? The checks below seem like they would belong in that Equals. #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We don't.
In reply to: 271578862 [](ancestors = 271578862)
@sfilipisfilipiApr 4, 2019•

edited

Loading

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'll open a bug about it.. we should have it.
In reply to: 271966741 [](ancestors = 271966741,271578862)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 /// </summary>
 private abstract class ValueMap
 {
 public readonly string KeyColumnName;
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

KeyColumnName [](start = 35, length = 13)
what do you think about using the entire column, rather than the name? we have a mix of storing indices, names, columns.. #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I will merge Name and Type into DataViewSchema.Column and clean redundant functions up.
In reply to: 271579703 [](ancestors = 271579703)
sfilipi
sfilipi
approved these changes
Apr 4, 2019
View reviewed changes
@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:
sfilipi
sfilipi
reviewed
Apr 4, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ConversionsExtensionsCatalog.cs
 return new ValueMappingEstimator<TInputType, TOutputType>(catalog.GetEnvironment(), lookupMap,
 lookupMap.Schema[ValueMappingTransformer.DefaultKeyColumnName],
 lookupMap.Schema[ValueMappingTransformer.DefaultValueColumnName],
 InputOutputColumnPair.ConvertToValueTuples(columns));
@sfilipisfilipiApr 4, 2019•

edited

Loading

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.

there is a bit of repetition here, does it make sense to make it part of the ctor? I mean create the lookup map inside the ctor, just pass the columns.
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I feel columns must go with the associated IDataView. There should be no column if its IDataView hasn't been created.
@wschin
wschin
merged commit b8a70ac
into
dotnet:masterApr 4, 2019
@wschin
wschin
deleted the

fix-value-map

branch
April 4, 2019 22:28
@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.


Reviewers


3 more reviewers

@Ivanidzo4kaIvanidzo4kaIvanidzo4ka left review comments
@sfilipisfilipisfilipi approved these changes
@singlissinglissinglis approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@wschinwschin

Labels

bugSomething isn't working

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

The ValueMap Train method assumes that keys are on the first column, and values on the second, rather than storing the columns and using those


4 participants

@wschin@Ivanidzo4ka@singlis@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
Fix a value-mapping bug - #3180
Merged
wschin merged 4 commits into
dotnet:masterfrom 
wschin:fix-value-map
Apr 4, 2019
Merged
Fix a value-mapping bug#3180
wschin merged 4 commits into
dotnet:masterfrom 
wschin:fix-value-map
Conversation
@wschin
@wschinwschin
commented
Apr 2, 2019

Copy link


Copy Markdown

Contributor
This PR fixes#3166.
@wschinwschin
added
the 
bug
Something isn't working
label
Apr 2, 2019
@wschinwschin
self-assigned this
Apr 2, 2019
@wschin
wschin
requested review from
sfilipi and singlisApril 2, 2019 21:43
@codecov
codecovBot
commented
Apr 2, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #3180 into master will increase coverage by 0.06%.

The diff coverage is 97.19%.
@@ Coverage Diff @@## master #3180 +/- ##
==========================================
+ Coverage 72.54% 72.6% +0.06% 
==========================================
Files 807 807 Lines 144774 145077 +303 Branches 16208 16213 +5 ==========================================
+ Hits 105021 105335 +314 + Misses 35339 35324 -15 - Partials 4414 4418 +4
FlagCoverage Δ
#Debug72.6% <97.19%> (+0.06%)⬆️
#production68.15% <91.42%> (+0.02%)⬆️
#test88.92% <100%> (+0.09%)⬆️
Impacted FilesCoverage Δ
...crosoft.ML.Tests/Transformers/ValueMappingTests.cs100% <100%> (ø)⬆️
src/Microsoft.ML.Data/Transforms/ValueMapping.cs84.26% <91.22%> (-0.14%)⬇️
...ML.Data/Transforms/ConversionsExtensionsCatalog.cs64.07% <92.3%> (+19.2%)⬆️
...c/Microsoft.ML.FastTree/Utils/ThreadTaskManager.cs79.48% <0%> (-20.52%)⬇️
src/Microsoft.ML.DataView/KeyDataViewType.cs74.57% <0%> (-3.76%)⬇️
src/Microsoft.ML.Maml/MAML.cs24.75% <0%> (-1.46%)⬇️
test/Microsoft.ML.Tests/ImagesTests.cs98.69% <0%> (-0.13%)⬇️
...Microsoft.ML.Tests/Transformers/NormalizerTests.cs100% <0%> (ø)⬆️
src/Microsoft.ML.Transforms/Text/TextCatalog.cs41.66% <0%> (ø)⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.26% <0%> (+0.15%)⬆️
... and 5 more
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
 /// <param name="keyColumn">Name of the key column in <paramref name="lookupMap"/>.</param>
 /// <param name="valueColumn">Name of the value column in <paramref name="lookupMap"/>.</param>
 /// <param name="columns">The list of names of the input columns to apply the transformation, and the name of the resulting column.</param>
 internal ValueMappingEstimator(IHostEnvironment env, IDataView lookupMap, DataViewSchema.Column keyColumn, DataViewSchema.Column valueColumn, params (string outputColumnName, string inputColumnName)[] columns)
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

If we are adding this constructor, is the one above (that takes a key column name and value column name) needed? #Resolved
@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.

+1, especially since the extension that calls into the ctor, takes in DataViewSchema.Column and passes down only the name of the column.
In reply to: 271533565 [](ancestors = 271533565)
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I will remove it in the next iteration.
In reply to: 271577343 [](ancestors = 271577343,271533565)
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 var valueType = dataView.Schema[valueIdx].Type;
 var valueMap = ValueMap.Create(keyType, valueType, ValueColumnMetadata);
 using (var cursor = dataView.GetRowCursor(dataView.Schema[keyIdx], dataView.Schema[valueIdx]))
 var retrievedKeyColumn = dataView.Schema[keyColumn.Index];
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

Schema [](start = 46, length = 6)
Can this be out of range? #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Will add a check in the next iteration.
In reply to: 271535033 [](ancestors = 271535033)
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 Host.Check(retrievedKeyColumn.Type == keyColumn.Type, nameof(keyColumn) + "'s column type doesn't match that of the associated column in " + nameof(dataView));
 Host.Check(retrievedKeyColumn.Annotations == keyColumn.Annotations, nameof(keyColumn) + "'s column annotions don't match those of the associated column in " + nameof(dataView));

 var retrievedValueColumn = dataView.Schema[valueColumn.Index];
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

same here. #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Will add a check as well.
In reply to: 271535097 [](ancestors = 271535097)
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 private readonly byte[] _dataView;

 private readonly DataViewSchema.Column _lookupKeyColumn;
 private readonly DataViewSchema.Column _lookupValueColumn;
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

private [](start = 8, length = 7)
Why store these? They look like they can be removed. #Resolved
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Super nice catch. Thanks.
In reply to: 271535842 [](ancestors = 271535842)
singlis
singlis
approved these changes
Apr 2, 2019
View reviewed changes
@singlissinglis
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:
Ivanidzo4ka
Ivanidzo4ka
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

@@ -46,7 +46,22 @@ public class ValueMappingEstimator : TrivialEstimator<ValueMappingTransformer>
 /// <param name="columns">The list of names of the input columns to apply the transformation, and the name of the resulting column.</param>
 internal ValueMappingEstimator(IHostEnvironment env, IDataView lookupMap, string keyColumn, string valueColumn, params (string outputColumnName, string inputColumnName)[] columns)
@Ivanidzo4kaIvanidzo4kaApr 3, 2019•

edited by sfilipi

Loading

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.

ValueMappingEstimator [](start = 17, length = 21)
I'm not getting why we still have this constructor

Public interface operates over Schema.Column object, any conversion from string to Schema.Column is unrelaable, why it's still around? #Closed
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Removed and related code is updated as well.
In reply to: 271544302 [](ancestors = 271544302)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
DataViewSchema.Column lookupKeyColumn, DataViewSchema.Column lookupValueColumn, (string outputColumnName, string inputColumnName)[] columns)
 : base(Contracts.CheckRef(env, nameof(env)).Register(nameof(ValueMappingTransformer)), columns)
 {
 Host.CheckNonEmpty(keyColumn, nameof(keyColumn), "A key column must be specified when passing in an IDataView for the value mapping");
@sfilipisfilipiApr 3, 2019•

edited

Loading

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.

Host.CheckNonEmpty [](start = 11, length = 19)
maybe check lookupKeyColumn.HasValue #Closed
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We can't. This is a struct, not a class.
In reply to: 271577590 [](ancestors = 271577590)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/Transformers/ValueMappingTests.cs
Outdated

 // Create a new ML context, for ML.NET operations. It can be used for exception tracking and logging, 
 // as well as the source of randomness.
 var mlContext = new MLContext();

@sfilipisfilipiApr 3, 2019•

edited

Loading

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.

you don't need this, the file should already have it: ML #Closed
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/Transformers/ValueMappingTests.cs
Outdated

 // Getting the resulting data as an IEnumerable.
 var features = mlContext.Data.CreateEnumerable<TransformedData>(transformedData, reuseRowObject: false).ToList();

 var expectedPrices = new float[] { 3.14f, 2000f, 1.19f, 2.17f, 33.784f };
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

expectedPrices [](start = 16, length = 14)
you have rawData storing those #Resolved
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yes. Thank you.
In reply to: 271578355 [](ancestors = 271578355)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
 _lookupValueColumn = lookupValueColumn;
 _valueMap = CreateValueMapFromDataView(lookupMap, _lookupKeyColumn, _lookupValueColumn);
 ValueColumnMetadata = lookupMap.Schema[_lookupValueColumn.Index].Annotations;

@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

Does _lookupValueColumn.Annotations work? #Resolved
@wschinwschinApr 4, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Just did a small refactorization around Annotations and extent a test to cover it at Iteration 4.
In reply to: 271578470 [](ancestors = 271578470)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 var retrievedKeyColumn = dataView.Schema[keyColumn.Index];
 Host.Check(retrievedKeyColumn.Index == keyColumn.Index, nameof(keyColumn) + "'s column index doesn't match that of the associated column in " + nameof(dataView));
 Host.Check(retrievedKeyColumn.Type == keyColumn.Type, nameof(keyColumn) + "'s column type doesn't match that of the associated column in " + nameof(dataView));
 Host.Check(retrievedKeyColumn.Annotations == keyColumn.Annotations, nameof(keyColumn) + "'s column annotions don't match those of the associated column in " + nameof(dataView));
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

annotions [](start = 111, length = 9)
typo #Resolved
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 var valueType = dataView.Schema[valueIdx].Type;
 var valueMap = ValueMap.Create(keyType, valueType, ValueColumnMetadata);
 using (var cursor = dataView.GetRowCursor(dataView.Schema[keyIdx], dataView.Schema[valueIdx]))
 var retrievedKeyColumn = dataView.Schema[keyColumn.Index];
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

retrievedKeyColumn [](start = 16, length = 18)
do we have any overload for Equals on the DataViewSchema.Column? The checks below seem like they would belong in that Equals. #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We don't.
In reply to: 271578862 [](ancestors = 271578862)
@sfilipisfilipiApr 4, 2019•

edited

Loading

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'll open a bug about it.. we should have it.
In reply to: 271966741 [](ancestors = 271966741,271578862)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 /// </summary>
 private abstract class ValueMap
 {
 public readonly string KeyColumnName;
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

KeyColumnName [](start = 35, length = 13)
what do you think about using the entire column, rather than the name? we have a mix of storing indices, names, columns.. #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I will merge Name and Type into DataViewSchema.Column and clean redundant functions up.
In reply to: 271579703 [](ancestors = 271579703)
sfilipi
sfilipi
approved these changes
Apr 4, 2019
View reviewed changes
@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:
sfilipi
sfilipi
reviewed
Apr 4, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ConversionsExtensionsCatalog.cs
 return new ValueMappingEstimator<TInputType, TOutputType>(catalog.GetEnvironment(), lookupMap,
 lookupMap.Schema[ValueMappingTransformer.DefaultKeyColumnName],
 lookupMap.Schema[ValueMappingTransformer.DefaultValueColumnName],
 InputOutputColumnPair.ConvertToValueTuples(columns));
@sfilipisfilipiApr 4, 2019•

edited

Loading

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.

there is a bit of repetition here, does it make sense to make it part of the ctor? I mean create the lookup map inside the ctor, just pass the columns.
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I feel columns must go with the associated IDataView. There should be no column if its IDataView hasn't been created.
@wschin
wschin
merged commit b8a70ac
into
dotnet:masterApr 4, 2019
@wschin
wschin
deleted the

fix-value-map

branch
April 4, 2019 22:28
@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.


Reviewers


3 more reviewers

@Ivanidzo4kaIvanidzo4kaIvanidzo4ka left review comments
@sfilipisfilipisfilipi approved these changes
@singlissinglissinglis approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@wschinwschin

Labels

bugSomething isn't working

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

The ValueMap Train method assumes that keys are on the first column, and values on the second, rather than storing the columns and using those


4 participants

@wschin@Ivanidzo4ka@singlis@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
Fix a value-mapping bug - #3180
Merged
wschin merged 4 commits into
dotnet:masterfrom 
wschin:fix-value-map
Apr 4, 2019
Merged
Fix a value-mapping bug#3180
wschin merged 4 commits into
dotnet:masterfrom 
wschin:fix-value-map
Conversation
@wschin
@wschinwschin
commented
Apr 2, 2019

Copy link


Copy Markdown

Contributor
This PR fixes#3166.
@wschinwschin
added
the 
bug
Something isn't working
label
Apr 2, 2019
@wschinwschin
self-assigned this
Apr 2, 2019
@wschin
wschin
requested review from
sfilipi and singlisApril 2, 2019 21:43
@codecov
codecovBot
commented
Apr 2, 2019•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #3180 into master will increase coverage by 0.06%.

The diff coverage is 97.19%.
@@ Coverage Diff @@## master #3180 +/- ##
==========================================
+ Coverage 72.54% 72.6% +0.06% 
==========================================
Files 807 807 Lines 144774 145077 +303 Branches 16208 16213 +5 ==========================================
+ Hits 105021 105335 +314 + Misses 35339 35324 -15 - Partials 4414 4418 +4
FlagCoverage Δ
#Debug72.6% <97.19%> (+0.06%)⬆️
#production68.15% <91.42%> (+0.02%)⬆️
#test88.92% <100%> (+0.09%)⬆️
Impacted FilesCoverage Δ
...crosoft.ML.Tests/Transformers/ValueMappingTests.cs100% <100%> (ø)⬆️
src/Microsoft.ML.Data/Transforms/ValueMapping.cs84.26% <91.22%> (-0.14%)⬇️
...ML.Data/Transforms/ConversionsExtensionsCatalog.cs64.07% <92.3%> (+19.2%)⬆️
...c/Microsoft.ML.FastTree/Utils/ThreadTaskManager.cs79.48% <0%> (-20.52%)⬇️
src/Microsoft.ML.DataView/KeyDataViewType.cs74.57% <0%> (-3.76%)⬇️
src/Microsoft.ML.Maml/MAML.cs24.75% <0%> (-1.46%)⬇️
test/Microsoft.ML.Tests/ImagesTests.cs98.69% <0%> (-0.13%)⬇️
...Microsoft.ML.Tests/Transformers/NormalizerTests.cs100% <0%> (ø)⬆️
src/Microsoft.ML.Transforms/Text/TextCatalog.cs41.66% <0%> (ø)⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.26% <0%> (+0.15%)⬆️
... and 5 more
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
 /// <param name="keyColumn">Name of the key column in <paramref name="lookupMap"/>.</param>
 /// <param name="valueColumn">Name of the value column in <paramref name="lookupMap"/>.</param>
 /// <param name="columns">The list of names of the input columns to apply the transformation, and the name of the resulting column.</param>
 internal ValueMappingEstimator(IHostEnvironment env, IDataView lookupMap, DataViewSchema.Column keyColumn, DataViewSchema.Column valueColumn, params (string outputColumnName, string inputColumnName)[] columns)
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

If we are adding this constructor, is the one above (that takes a key column name and value column name) needed? #Resolved
@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.

+1, especially since the extension that calls into the ctor, takes in DataViewSchema.Column and passes down only the name of the column.
In reply to: 271533565 [](ancestors = 271533565)
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I will remove it in the next iteration.
In reply to: 271577343 [](ancestors = 271577343,271533565)
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 var valueType = dataView.Schema[valueIdx].Type;
 var valueMap = ValueMap.Create(keyType, valueType, ValueColumnMetadata);
 using (var cursor = dataView.GetRowCursor(dataView.Schema[keyIdx], dataView.Schema[valueIdx]))
 var retrievedKeyColumn = dataView.Schema[keyColumn.Index];
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

Schema [](start = 46, length = 6)
Can this be out of range? #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Will add a check in the next iteration.
In reply to: 271535033 [](ancestors = 271535033)
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 Host.Check(retrievedKeyColumn.Type == keyColumn.Type, nameof(keyColumn) + "'s column type doesn't match that of the associated column in " + nameof(dataView));
 Host.Check(retrievedKeyColumn.Annotations == keyColumn.Annotations, nameof(keyColumn) + "'s column annotions don't match those of the associated column in " + nameof(dataView));

 var retrievedValueColumn = dataView.Schema[valueColumn.Index];
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

same here. #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Will add a check as well.
In reply to: 271535097 [](ancestors = 271535097)
singlis
singlis
reviewed
Apr 2, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 private readonly byte[] _dataView;

 private readonly DataViewSchema.Column _lookupKeyColumn;
 private readonly DataViewSchema.Column _lookupValueColumn;
@singlissinglisApr 2, 2019•

edited by wschin

Loading

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.

private [](start = 8, length = 7)
Why store these? They look like they can be removed. #Resolved
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Super nice catch. Thanks.
In reply to: 271535842 [](ancestors = 271535842)
singlis
singlis
approved these changes
Apr 2, 2019
View reviewed changes
@singlissinglis
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:
Ivanidzo4ka
Ivanidzo4ka
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

@@ -46,7 +46,22 @@ public class ValueMappingEstimator : TrivialEstimator<ValueMappingTransformer>
 /// <param name="columns">The list of names of the input columns to apply the transformation, and the name of the resulting column.</param>
 internal ValueMappingEstimator(IHostEnvironment env, IDataView lookupMap, string keyColumn, string valueColumn, params (string outputColumnName, string inputColumnName)[] columns)
@Ivanidzo4kaIvanidzo4kaApr 3, 2019•

edited by sfilipi

Loading

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.

ValueMappingEstimator [](start = 17, length = 21)
I'm not getting why we still have this constructor

Public interface operates over Schema.Column object, any conversion from string to Schema.Column is unrelaable, why it's still around? #Closed
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Removed and related code is updated as well.
In reply to: 271544302 [](ancestors = 271544302)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
DataViewSchema.Column lookupKeyColumn, DataViewSchema.Column lookupValueColumn, (string outputColumnName, string inputColumnName)[] columns)
 : base(Contracts.CheckRef(env, nameof(env)).Register(nameof(ValueMappingTransformer)), columns)
 {
 Host.CheckNonEmpty(keyColumn, nameof(keyColumn), "A key column must be specified when passing in an IDataView for the value mapping");
@sfilipisfilipiApr 3, 2019•

edited

Loading

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.

Host.CheckNonEmpty [](start = 11, length = 19)
maybe check lookupKeyColumn.HasValue #Closed
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We can't. This is a struct, not a class.
In reply to: 271577590 [](ancestors = 271577590)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/Transformers/ValueMappingTests.cs
Outdated

 // Create a new ML context, for ML.NET operations. It can be used for exception tracking and logging, 
 // as well as the source of randomness.
 var mlContext = new MLContext();

@sfilipisfilipiApr 3, 2019•

edited

Loading

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.

you don't need this, the file should already have it: ML #Closed
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/Transformers/ValueMappingTests.cs
Outdated

 // Getting the resulting data as an IEnumerable.
 var features = mlContext.Data.CreateEnumerable<TransformedData>(transformedData, reuseRowObject: false).ToList();

 var expectedPrices = new float[] { 3.14f, 2000f, 1.19f, 2.17f, 33.784f };
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

expectedPrices [](start = 16, length = 14)
you have rawData storing those #Resolved
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yes. Thank you.
In reply to: 271578355 [](ancestors = 271578355)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
 _lookupValueColumn = lookupValueColumn;
 _valueMap = CreateValueMapFromDataView(lookupMap, _lookupKeyColumn, _lookupValueColumn);
 ValueColumnMetadata = lookupMap.Schema[_lookupValueColumn.Index].Annotations;

@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

Does _lookupValueColumn.Annotations work? #Resolved
@wschinwschinApr 4, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Just did a small refactorization around Annotations and extent a test to cover it at Iteration 4.
In reply to: 271578470 [](ancestors = 271578470)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 var retrievedKeyColumn = dataView.Schema[keyColumn.Index];
 Host.Check(retrievedKeyColumn.Index == keyColumn.Index, nameof(keyColumn) + "'s column index doesn't match that of the associated column in " + nameof(dataView));
 Host.Check(retrievedKeyColumn.Type == keyColumn.Type, nameof(keyColumn) + "'s column type doesn't match that of the associated column in " + nameof(dataView));
 Host.Check(retrievedKeyColumn.Annotations == keyColumn.Annotations, nameof(keyColumn) + "'s column annotions don't match those of the associated column in " + nameof(dataView));
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

annotions [](start = 111, length = 9)
typo #Resolved
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 var valueType = dataView.Schema[valueIdx].Type;
 var valueMap = ValueMap.Create(keyType, valueType, ValueColumnMetadata);
 using (var cursor = dataView.GetRowCursor(dataView.Schema[keyIdx], dataView.Schema[valueIdx]))
 var retrievedKeyColumn = dataView.Schema[keyColumn.Index];
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

retrievedKeyColumn [](start = 16, length = 18)
do we have any overload for Equals on the DataViewSchema.Column? The checks below seem like they would belong in that Equals. #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We don't.
In reply to: 271578862 [](ancestors = 271578862)
@sfilipisfilipiApr 4, 2019•

edited

Loading

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'll open a bug about it.. we should have it.
In reply to: 271966741 [](ancestors = 271966741,271578862)
sfilipi
sfilipi
reviewed
Apr 3, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ValueMapping.cs
Outdated

 /// </summary>
 private abstract class ValueMap
 {
 public readonly string KeyColumnName;
@sfilipisfilipiApr 3, 2019•

edited by wschin

Loading

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.

KeyColumnName [](start = 35, length = 13)
what do you think about using the entire column, rather than the name? we have a mix of storing indices, names, columns.. #Resolved
@wschinwschinApr 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I will merge Name and Type into DataViewSchema.Column and clean redundant functions up.
In reply to: 271579703 [](ancestors = 271579703)
sfilipi
sfilipi
approved these changes
Apr 4, 2019
View reviewed changes
@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:
sfilipi
sfilipi
reviewed
Apr 4, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.Data/Transforms/ConversionsExtensionsCatalog.cs
 return new ValueMappingEstimator<TInputType, TOutputType>(catalog.GetEnvironment(), lookupMap,
 lookupMap.Schema[ValueMappingTransformer.DefaultKeyColumnName],
 lookupMap.Schema[ValueMappingTransformer.DefaultValueColumnName],
 InputOutputColumnPair.ConvertToValueTuples(columns));
@sfilipisfilipiApr 4, 2019•

edited

Loading

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.

there is a bit of repetition here, does it make sense to make it part of the ctor? I mean create the lookup map inside the ctor, just pass the columns.
@wschinwschinApr 4, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I feel columns must go with the associated IDataView. There should be no column if its IDataView hasn't been created.
@wschin
wschin
merged commit b8a70ac
into
dotnet:masterApr 4, 2019
@wschin
wschin
deleted the

fix-value-map

branch
April 4, 2019 22:28
@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.


Reviewers


3 more reviewers

@Ivanidzo4kaIvanidzo4kaIvanidzo4ka left review comments
@sfilipisfilipisfilipi approved these changes
@singlissinglissinglis approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@wschinwschin

Labels

bugSomething isn't working

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

The ValueMap Train method assumes that keys are on the first column, and values on the second, rather than storing the columns and using those


4 participants

@wschin@Ivanidzo4ka@singlis@sfilipi