Multiple feature columns in FFM - #2205

Merged
wschin merged 5 commits into
dotnet:masterfrom
wschin:adv-ffm-args
Jan 23, 2019
Merged

Multiple feature columns in FFM#2205
wschin merged 5 commits into
dotnet:masterfrom
wschin:adv-ffm-args

Conversation

@wschin

@wschinwschin commented Jan 22, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

This PR somehow provides a solution to #2179 regarding FFM via allowing multiple feature column names in advanced trainer arguments.

Strategy: adding one extra field to Arguments of FFM; that filed is

/// <summary>/// Extra feature column names. The column named <see cref="LearnerInputBase.FeatureColumn"/> stores features from the first field./// The i-th string in <see cref="ExtraFeatureColumns"/> stores the name of the (i+1)-th field's feature column./// </summary>[Argument(ArgumentType.Multiple,HelpText="Extra columns to use for feature vectors. The i-th specified string denotes the column containing features form the (i+1)-th field."+" Note that the first field is specified by \"feat\" instead of \"exfeat\".",ShortName="exfeat",SortOrder=7)]publicstring[]ExtraFeatureColumns;

@wschinwschin self-assigned this Jan 22, 2019
abgoswam
abgoswam reviewed Jan 22, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.SamplesUtils/SamplesDatasetUtils.cs
Field1 = new float[_simpleBinaryClassSampleFeatureLength],
Field2 = new float[_simpleBinaryClassSampleFeatureLength] };
// Fill feature vector according the assigned label.
for (int j = 0; j < 10; ++j)

@abgoswamabgoswamJan 22, 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.

10 [](start = 36, length = 2)

_simpleBinaryClassSampleFeatureLength ? #Closed

@wschinwschinJan 22, 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.

Nice catch. Fixed.

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

abgoswam
abgoswam reviewed Jan 22, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.SamplesUtils/SamplesDatasetUtils.cs
{
public bool Label;

[VectorType(_simpleBinaryClassSampleFeatureLength)]

@abgoswamabgoswamJan 22, 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.

VectorType [](start = 13, length = 10)

am curious - is this attribute required ? #Closed

@wschinwschinJan 22, 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.

It is required.

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

abgoswam
abgoswam previously approved these changes Jan 22, 2019
View reviewed changes

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

wschin
wschin commented Jan 23, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.FastTree/TreeEnsemble/TreeEnsemble.cs Outdated
@abgoswam
abgoswam dismissed their stale reviewJanuary 23, 2019 18:01

revoking review

abgoswam
abgoswam reviewed Jan 23, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/TrainerEstimators/FAFMEstimator.cs

// Customized the field names.
ffmArgs.FeatureColumn = nameof(DatasetUtils.FfmExample.Field0); // First field.
ffmArgs.ExtraFeatureColumns = new[]{ nameof(DatasetUtils.FfmExample.Field1), nameof(DatasetUtils.FfmExample.Field2) };

@abgoswamabgoswamJan 23, 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.

ExtraFeatureColumns [](start = 20, length = 19)

this looks slightly odd . isn't it ?

am curious -- why move away from the convention used in iteration #2, where we were re-defining FeatureColumn as a string[]

@wschinwschinJan 23, 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.

Framework of generating entry points generates the hidden field (old feature column name) and therefore we have two fields with the same name and an error.

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

@abgoswam

abgoswam commented Jan 23, 2019

Copy link
Copy Markdown
Member
 public readonly SchemaShape.Column[] FeatureColumns;

internal.

We do not want this exposed especially since it holds content from Options.FeatureColumn and Options.ExtraFeatureColumns

Refers to: src/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs:93 in e454a19. [](commit_id = e454a19, deletion_comment = False)

abgoswam
abgoswam approved these changes Jan 23, 2019
View reviewed changes

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

shmoradims
shmoradims reviewed Jan 23, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs
FeatureColumns[0] = new SchemaShape.Column(args.FeatureColumn, SchemaShape.Column.VectorKind.Vector, NumberType.R4, false);

// Add 2nd, 3rd, and other fields from a FFM-specific argument, args.ExtraFeatureColumns.
for (int i = 0; args.ExtraFeatureColumns != null && i < args.ExtraFeatureColumns.Length; i++)

@shmoradimsshmoradimsJan 23, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

for (int i = 0; args.ExtraFeatureColumns != null && i < args.ExtraFeatureColumns.Length; i++) [](start = 12, length = 93)

could this be a simple foreach loop instead?

@wschinwschinJan 23, 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 guess it can't. The array size is pre-defined and I prefer not to create an intermediate list just for calling Add.

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

@wschin

wschin commented Jan 23, 2019

Copy link
Copy Markdown
ContributorAuthor
 public readonly SchemaShape.Column[] FeatureColumns;

It's not related to Options. It's a state of a trainer which can be specified by multiple (I guess two) ways.

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

Refers to: src/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs:93 in e454a19. [](commit_id = e454a19, deletion_comment = False)

shmoradims
shmoradims approved these changes Jan 23, 2019
View reviewed changes

@shmoradimsshmoradims left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

:shipit:

@wschin
wschin merged commit 053062b into dotnet:masterJan 23, 2019
@wschin
wschin deleted the adv-ffm-args branch January 23, 2019 18:38
@abgoswamabgoswam mentioned this pull request Jan 28, 2019
Closed
@ghostghost locked as resolved and limited conversation to collaborators Mar 25, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sfilipisfilipiAwaiting requested review from sfilipi
2 more reviewers
@abgoswamabgoswamabgoswam approved these changes
@shmoradimsshmoradimsshmoradims approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@wschinwschin

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@wschin@abgoswam@shmoradims
, '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
Multiple feature columns in FFM - #2205
Merged
wschin merged 5 commits into
dotnet:masterfrom 
wschin:adv-ffm-args
Jan 23, 2019
Merged
Multiple feature columns in FFM#2205
wschin merged 5 commits into
dotnet:masterfrom 
wschin:adv-ffm-args
Conversation
@wschin
@wschinwschin
commented
Jan 22, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR somehow provides a solution to #2179 regarding FFM via allowing multiple feature column names in advanced trainer arguments.
Strategy: adding one extra field to Arguments of FFM; that filed is
/// <summary>/// Extra feature column names. The column named <see cref="LearnerInputBase.FeatureColumn"/> stores features from the first field./// The i-th string in <see cref="ExtraFeatureColumns"/> stores the name of the (i+1)-th field's feature column./// </summary>[Argument(ArgumentType.Multiple,HelpText="Extra columns to use for feature vectors. The i-th specified string denotes the column containing features form the (i+1)-th field."+" Note that the first field is specified by \"feat\" instead of \"exfeat\".",ShortName="exfeat",SortOrder=7)]publicstring[]ExtraFeatureColumns;
@wschinwschin
self-assigned this
Jan 22, 2019
abgoswam
abgoswam
reviewed
Jan 22, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.SamplesUtils/SamplesDatasetUtils.cs
 Field1 = new float[_simpleBinaryClassSampleFeatureLength],
 Field2 = new float[_simpleBinaryClassSampleFeatureLength] };
 // Fill feature vector according the assigned label.
 for (int j = 0; j < 10; ++j)
@abgoswamabgoswamJan 22, 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.

10 [](start = 36, length = 2)
_simpleBinaryClassSampleFeatureLength ? #Closed
@wschinwschinJan 22, 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.

Nice catch. Fixed.
In reply to: 249993941 [](ancestors = 249993941)
abgoswam
abgoswam
reviewed
Jan 22, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.SamplesUtils/SamplesDatasetUtils.cs
 {
 public bool Label;

 [VectorType(_simpleBinaryClassSampleFeatureLength)]
@abgoswamabgoswamJan 22, 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.

VectorType [](start = 13, length = 10)
am curious - is this attribute required ? #Closed
@wschinwschinJan 22, 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.

It is required.
In reply to: 249994977 [](ancestors = 249994977)
abgoswam
abgoswam
previously approved these changes
Jan 22, 2019
View reviewed changes
@abgoswamabgoswam
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:
wschin
wschin
commented
Jan 23, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.FastTree/TreeEnsemble/TreeEnsemble.cs
Outdated

@abgoswam
abgoswam
dismissed
their stale reviewJanuary 23, 2019 18:01
revoking review
abgoswam
abgoswam
reviewed
Jan 23, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/TrainerEstimators/FAFMEstimator.cs

 // Customized the field names.
 ffmArgs.FeatureColumn = nameof(DatasetUtils.FfmExample.Field0); // First field.
 ffmArgs.ExtraFeatureColumns = new[]{ nameof(DatasetUtils.FfmExample.Field1), nameof(DatasetUtils.FfmExample.Field2) };
@abgoswamabgoswamJan 23, 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.

ExtraFeatureColumns [](start = 20, length = 19)
this looks slightly odd . isn't it ?
am curious -- why move away from the convention used in iteration #2, where we were re-defining FeatureColumn as a string[]
@wschinwschinJan 23, 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.

Framework of generating entry points generates the hidden field (old feature column name) and therefore we have two fields with the same name and an error.
In reply to: 250309618 [](ancestors = 250309618)
@abgoswam
abgoswam
commented
Jan 23, 2019

Copy link


Copy Markdown

Member
 public readonly SchemaShape.Column[] FeatureColumns;

internal.
We do not want this exposed especially since it holds content from Options.FeatureColumn and Options.ExtraFeatureColumns
Refers to: src/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs:93 in e454a19. [](commit_id = e454a19, deletion_comment = False)
abgoswam
abgoswam
approved these changes
Jan 23, 2019
View reviewed changes
@abgoswamabgoswam
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:
shmoradims
shmoradims
reviewed
Jan 23, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs
 FeatureColumns[0] = new SchemaShape.Column(args.FeatureColumn, SchemaShape.Column.VectorKind.Vector, NumberType.R4, false);

 // Add 2nd, 3rd, and other fields from a FFM-specific argument, args.ExtraFeatureColumns.
 for (int i = 0; args.ExtraFeatureColumns != null && i < args.ExtraFeatureColumns.Length; i++)
@shmoradimsshmoradimsJan 23, 2019

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

for (int i = 0; args.ExtraFeatureColumns != null && i < args.ExtraFeatureColumns.Length; i++) [](start = 12, length = 93)
could this be a simple foreach loop instead?
@wschinwschinJan 23, 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 guess it can't. The array size is pre-defined and I prefer not to create an intermediate list just for calling Add.
In reply to: 250316802 [](ancestors = 250316802)
@wschin
wschin
commented
Jan 23, 2019

Copy link


Copy Markdown

ContributorAuthor
 public readonly SchemaShape.Column[] FeatureColumns;

It's not related to Options. It's a state of a trainer which can be specified by multiple (I guess two) ways.
In reply to: 456913047 [](ancestors = 456913047)
Refers to: src/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs:93 in e454a19. [](commit_id = e454a19, deletion_comment = False)
shmoradims
shmoradims
approved these changes
Jan 23, 2019
View reviewed changes
@shmoradimsshmoradims
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

:shipit:
@wschin
wschin
merged commit 053062b
into
dotnet:masterJan 23, 2019
@wschin
wschin
deleted the

adv-ffm-args

branch
January 23, 2019 18:38
@abgoswamabgoswam
mentioned this pull request
Jan 28, 2019

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 25, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@sfilipisfilipiAwaiting requested review from sfilipi

2 more reviewers

@abgoswamabgoswamabgoswam approved these changes
@shmoradimsshmoradimsshmoradims approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@wschinwschin

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@wschin@abgoswam@shmoradims

, '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
Multiple feature columns in FFM - #2205
Merged
wschin merged 5 commits into
dotnet:masterfrom 
wschin:adv-ffm-args
Jan 23, 2019
Merged
Multiple feature columns in FFM#2205
wschin merged 5 commits into
dotnet:masterfrom 
wschin:adv-ffm-args
Conversation
@wschin
@wschinwschin
commented
Jan 22, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR somehow provides a solution to #2179 regarding FFM via allowing multiple feature column names in advanced trainer arguments.
Strategy: adding one extra field to Arguments of FFM; that filed is
/// <summary>/// Extra feature column names. The column named <see cref="LearnerInputBase.FeatureColumn"/> stores features from the first field./// The i-th string in <see cref="ExtraFeatureColumns"/> stores the name of the (i+1)-th field's feature column./// </summary>[Argument(ArgumentType.Multiple,HelpText="Extra columns to use for feature vectors. The i-th specified string denotes the column containing features form the (i+1)-th field."+" Note that the first field is specified by \"feat\" instead of \"exfeat\".",ShortName="exfeat",SortOrder=7)]publicstring[]ExtraFeatureColumns;
@wschinwschin
self-assigned this
Jan 22, 2019
abgoswam
abgoswam
reviewed
Jan 22, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.SamplesUtils/SamplesDatasetUtils.cs
 Field1 = new float[_simpleBinaryClassSampleFeatureLength],
 Field2 = new float[_simpleBinaryClassSampleFeatureLength] };
 // Fill feature vector according the assigned label.
 for (int j = 0; j < 10; ++j)
@abgoswamabgoswamJan 22, 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.

10 [](start = 36, length = 2)
_simpleBinaryClassSampleFeatureLength ? #Closed
@wschinwschinJan 22, 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.

Nice catch. Fixed.
In reply to: 249993941 [](ancestors = 249993941)
abgoswam
abgoswam
reviewed
Jan 22, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.SamplesUtils/SamplesDatasetUtils.cs
 {
 public bool Label;

 [VectorType(_simpleBinaryClassSampleFeatureLength)]
@abgoswamabgoswamJan 22, 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.

VectorType [](start = 13, length = 10)
am curious - is this attribute required ? #Closed
@wschinwschinJan 22, 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.

It is required.
In reply to: 249994977 [](ancestors = 249994977)
abgoswam
abgoswam
previously approved these changes
Jan 22, 2019
View reviewed changes
@abgoswamabgoswam
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:
wschin
wschin
commented
Jan 23, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.FastTree/TreeEnsemble/TreeEnsemble.cs
Outdated

@abgoswam
abgoswam
dismissed
their stale reviewJanuary 23, 2019 18:01
revoking review
abgoswam
abgoswam
reviewed
Jan 23, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/TrainerEstimators/FAFMEstimator.cs

 // Customized the field names.
 ffmArgs.FeatureColumn = nameof(DatasetUtils.FfmExample.Field0); // First field.
 ffmArgs.ExtraFeatureColumns = new[]{ nameof(DatasetUtils.FfmExample.Field1), nameof(DatasetUtils.FfmExample.Field2) };
@abgoswamabgoswamJan 23, 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.

ExtraFeatureColumns [](start = 20, length = 19)
this looks slightly odd . isn't it ?
am curious -- why move away from the convention used in iteration #2, where we were re-defining FeatureColumn as a string[]
@wschinwschinJan 23, 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.

Framework of generating entry points generates the hidden field (old feature column name) and therefore we have two fields with the same name and an error.
In reply to: 250309618 [](ancestors = 250309618)
@abgoswam
abgoswam
commented
Jan 23, 2019

Copy link


Copy Markdown

Member
 public readonly SchemaShape.Column[] FeatureColumns;

internal.
We do not want this exposed especially since it holds content from Options.FeatureColumn and Options.ExtraFeatureColumns
Refers to: src/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs:93 in e454a19. [](commit_id = e454a19, deletion_comment = False)
abgoswam
abgoswam
approved these changes
Jan 23, 2019
View reviewed changes
@abgoswamabgoswam
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:
shmoradims
shmoradims
reviewed
Jan 23, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs
 FeatureColumns[0] = new SchemaShape.Column(args.FeatureColumn, SchemaShape.Column.VectorKind.Vector, NumberType.R4, false);

 // Add 2nd, 3rd, and other fields from a FFM-specific argument, args.ExtraFeatureColumns.
 for (int i = 0; args.ExtraFeatureColumns != null && i < args.ExtraFeatureColumns.Length; i++)
@shmoradimsshmoradimsJan 23, 2019

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

for (int i = 0; args.ExtraFeatureColumns != null && i < args.ExtraFeatureColumns.Length; i++) [](start = 12, length = 93)
could this be a simple foreach loop instead?
@wschinwschinJan 23, 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 guess it can't. The array size is pre-defined and I prefer not to create an intermediate list just for calling Add.
In reply to: 250316802 [](ancestors = 250316802)
@wschin
wschin
commented
Jan 23, 2019

Copy link


Copy Markdown

ContributorAuthor
 public readonly SchemaShape.Column[] FeatureColumns;

It's not related to Options. It's a state of a trainer which can be specified by multiple (I guess two) ways.
In reply to: 456913047 [](ancestors = 456913047)
Refers to: src/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs:93 in e454a19. [](commit_id = e454a19, deletion_comment = False)
shmoradims
shmoradims
approved these changes
Jan 23, 2019
View reviewed changes
@shmoradimsshmoradims
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

:shipit:
@wschin
wschin
merged commit 053062b
into
dotnet:masterJan 23, 2019
@wschin
wschin
deleted the

adv-ffm-args

branch
January 23, 2019 18:38
@abgoswamabgoswam
mentioned this pull request
Jan 28, 2019

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 25, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@sfilipisfilipiAwaiting requested review from sfilipi

2 more reviewers

@abgoswamabgoswamabgoswam approved these changes
@shmoradimsshmoradimsshmoradims approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@wschinwschin

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@wschin@abgoswam@shmoradims

, '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
Multiple feature columns in FFM - #2205
Merged
wschin merged 5 commits into
dotnet:masterfrom 
wschin:adv-ffm-args
Jan 23, 2019
Merged
Multiple feature columns in FFM#2205
wschin merged 5 commits into
dotnet:masterfrom 
wschin:adv-ffm-args
Conversation
@wschin
@wschinwschin
commented
Jan 22, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR somehow provides a solution to #2179 regarding FFM via allowing multiple feature column names in advanced trainer arguments.
Strategy: adding one extra field to Arguments of FFM; that filed is
/// <summary>/// Extra feature column names. The column named <see cref="LearnerInputBase.FeatureColumn"/> stores features from the first field./// The i-th string in <see cref="ExtraFeatureColumns"/> stores the name of the (i+1)-th field's feature column./// </summary>[Argument(ArgumentType.Multiple,HelpText="Extra columns to use for feature vectors. The i-th specified string denotes the column containing features form the (i+1)-th field."+" Note that the first field is specified by \"feat\" instead of \"exfeat\".",ShortName="exfeat",SortOrder=7)]publicstring[]ExtraFeatureColumns;
@wschinwschin
self-assigned this
Jan 22, 2019
abgoswam
abgoswam
reviewed
Jan 22, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.SamplesUtils/SamplesDatasetUtils.cs
 Field1 = new float[_simpleBinaryClassSampleFeatureLength],
 Field2 = new float[_simpleBinaryClassSampleFeatureLength] };
 // Fill feature vector according the assigned label.
 for (int j = 0; j < 10; ++j)
@abgoswamabgoswamJan 22, 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.

10 [](start = 36, length = 2)
_simpleBinaryClassSampleFeatureLength ? #Closed
@wschinwschinJan 22, 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.

Nice catch. Fixed.
In reply to: 249993941 [](ancestors = 249993941)
abgoswam
abgoswam
reviewed
Jan 22, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.SamplesUtils/SamplesDatasetUtils.cs
 {
 public bool Label;

 [VectorType(_simpleBinaryClassSampleFeatureLength)]
@abgoswamabgoswamJan 22, 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.

VectorType [](start = 13, length = 10)
am curious - is this attribute required ? #Closed
@wschinwschinJan 22, 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.

It is required.
In reply to: 249994977 [](ancestors = 249994977)
abgoswam
abgoswam
previously approved these changes
Jan 22, 2019
View reviewed changes
@abgoswamabgoswam
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:
wschin
wschin
commented
Jan 23, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.FastTree/TreeEnsemble/TreeEnsemble.cs
Outdated

@abgoswam
abgoswam
dismissed
their stale reviewJanuary 23, 2019 18:01
revoking review
abgoswam
abgoswam
reviewed
Jan 23, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/TrainerEstimators/FAFMEstimator.cs

 // Customized the field names.
 ffmArgs.FeatureColumn = nameof(DatasetUtils.FfmExample.Field0); // First field.
 ffmArgs.ExtraFeatureColumns = new[]{ nameof(DatasetUtils.FfmExample.Field1), nameof(DatasetUtils.FfmExample.Field2) };
@abgoswamabgoswamJan 23, 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.

ExtraFeatureColumns [](start = 20, length = 19)
this looks slightly odd . isn't it ?
am curious -- why move away from the convention used in iteration #2, where we were re-defining FeatureColumn as a string[]
@wschinwschinJan 23, 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.

Framework of generating entry points generates the hidden field (old feature column name) and therefore we have two fields with the same name and an error.
In reply to: 250309618 [](ancestors = 250309618)
@abgoswam
abgoswam
commented
Jan 23, 2019

Copy link


Copy Markdown

Member
 public readonly SchemaShape.Column[] FeatureColumns;

internal.
We do not want this exposed especially since it holds content from Options.FeatureColumn and Options.ExtraFeatureColumns
Refers to: src/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs:93 in e454a19. [](commit_id = e454a19, deletion_comment = False)
abgoswam
abgoswam
approved these changes
Jan 23, 2019
View reviewed changes
@abgoswamabgoswam
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:
shmoradims
shmoradims
reviewed
Jan 23, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs
 FeatureColumns[0] = new SchemaShape.Column(args.FeatureColumn, SchemaShape.Column.VectorKind.Vector, NumberType.R4, false);

 // Add 2nd, 3rd, and other fields from a FFM-specific argument, args.ExtraFeatureColumns.
 for (int i = 0; args.ExtraFeatureColumns != null && i < args.ExtraFeatureColumns.Length; i++)
@shmoradimsshmoradimsJan 23, 2019

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

for (int i = 0; args.ExtraFeatureColumns != null && i < args.ExtraFeatureColumns.Length; i++) [](start = 12, length = 93)
could this be a simple foreach loop instead?
@wschinwschinJan 23, 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 guess it can't. The array size is pre-defined and I prefer not to create an intermediate list just for calling Add.
In reply to: 250316802 [](ancestors = 250316802)
@wschin
wschin
commented
Jan 23, 2019

Copy link


Copy Markdown

ContributorAuthor
 public readonly SchemaShape.Column[] FeatureColumns;

It's not related to Options. It's a state of a trainer which can be specified by multiple (I guess two) ways.
In reply to: 456913047 [](ancestors = 456913047)
Refers to: src/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs:93 in e454a19. [](commit_id = e454a19, deletion_comment = False)
shmoradims
shmoradims
approved these changes
Jan 23, 2019
View reviewed changes
@shmoradimsshmoradims
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

:shipit:
@wschin
wschin
merged commit 053062b
into
dotnet:masterJan 23, 2019
@wschin
wschin
deleted the

adv-ffm-args

branch
January 23, 2019 18:38
@abgoswamabgoswam
mentioned this pull request
Jan 28, 2019

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 25, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@sfilipisfilipiAwaiting requested review from sfilipi

2 more reviewers

@abgoswamabgoswamabgoswam approved these changes
@shmoradimsshmoradimsshmoradims approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@wschinwschin

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@wschin@abgoswam@shmoradims

, '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
Multiple feature columns in FFM - #2205
Merged
wschin merged 5 commits into
dotnet:masterfrom 
wschin:adv-ffm-args
Jan 23, 2019
Merged
Multiple feature columns in FFM#2205
wschin merged 5 commits into
dotnet:masterfrom 
wschin:adv-ffm-args
Conversation
@wschin
@wschinwschin
commented
Jan 22, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR somehow provides a solution to #2179 regarding FFM via allowing multiple feature column names in advanced trainer arguments.
Strategy: adding one extra field to Arguments of FFM; that filed is
/// <summary>/// Extra feature column names. The column named <see cref="LearnerInputBase.FeatureColumn"/> stores features from the first field./// The i-th string in <see cref="ExtraFeatureColumns"/> stores the name of the (i+1)-th field's feature column./// </summary>[Argument(ArgumentType.Multiple,HelpText="Extra columns to use for feature vectors. The i-th specified string denotes the column containing features form the (i+1)-th field."+" Note that the first field is specified by \"feat\" instead of \"exfeat\".",ShortName="exfeat",SortOrder=7)]publicstring[]ExtraFeatureColumns;
@wschinwschin
self-assigned this
Jan 22, 2019
abgoswam
abgoswam
reviewed
Jan 22, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.SamplesUtils/SamplesDatasetUtils.cs
 Field1 = new float[_simpleBinaryClassSampleFeatureLength],
 Field2 = new float[_simpleBinaryClassSampleFeatureLength] };
 // Fill feature vector according the assigned label.
 for (int j = 0; j < 10; ++j)
@abgoswamabgoswamJan 22, 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.

10 [](start = 36, length = 2)
_simpleBinaryClassSampleFeatureLength ? #Closed
@wschinwschinJan 22, 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.

Nice catch. Fixed.
In reply to: 249993941 [](ancestors = 249993941)
abgoswam
abgoswam
reviewed
Jan 22, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.SamplesUtils/SamplesDatasetUtils.cs
 {
 public bool Label;

 [VectorType(_simpleBinaryClassSampleFeatureLength)]
@abgoswamabgoswamJan 22, 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.

VectorType [](start = 13, length = 10)
am curious - is this attribute required ? #Closed
@wschinwschinJan 22, 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.

It is required.
In reply to: 249994977 [](ancestors = 249994977)
abgoswam
abgoswam
previously approved these changes
Jan 22, 2019
View reviewed changes
@abgoswamabgoswam
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:
wschin
wschin
commented
Jan 23, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.FastTree/TreeEnsemble/TreeEnsemble.cs
Outdated

@abgoswam
abgoswam
dismissed
their stale reviewJanuary 23, 2019 18:01
revoking review
abgoswam
abgoswam
reviewed
Jan 23, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/TrainerEstimators/FAFMEstimator.cs

 // Customized the field names.
 ffmArgs.FeatureColumn = nameof(DatasetUtils.FfmExample.Field0); // First field.
 ffmArgs.ExtraFeatureColumns = new[]{ nameof(DatasetUtils.FfmExample.Field1), nameof(DatasetUtils.FfmExample.Field2) };
@abgoswamabgoswamJan 23, 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.

ExtraFeatureColumns [](start = 20, length = 19)
this looks slightly odd . isn't it ?
am curious -- why move away from the convention used in iteration #2, where we were re-defining FeatureColumn as a string[]
@wschinwschinJan 23, 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.

Framework of generating entry points generates the hidden field (old feature column name) and therefore we have two fields with the same name and an error.
In reply to: 250309618 [](ancestors = 250309618)
@abgoswam
abgoswam
commented
Jan 23, 2019

Copy link


Copy Markdown

Member
 public readonly SchemaShape.Column[] FeatureColumns;

internal.
We do not want this exposed especially since it holds content from Options.FeatureColumn and Options.ExtraFeatureColumns
Refers to: src/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs:93 in e454a19. [](commit_id = e454a19, deletion_comment = False)
abgoswam
abgoswam
approved these changes
Jan 23, 2019
View reviewed changes
@abgoswamabgoswam
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:
shmoradims
shmoradims
reviewed
Jan 23, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs
 FeatureColumns[0] = new SchemaShape.Column(args.FeatureColumn, SchemaShape.Column.VectorKind.Vector, NumberType.R4, false);

 // Add 2nd, 3rd, and other fields from a FFM-specific argument, args.ExtraFeatureColumns.
 for (int i = 0; args.ExtraFeatureColumns != null && i < args.ExtraFeatureColumns.Length; i++)
@shmoradimsshmoradimsJan 23, 2019

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

for (int i = 0; args.ExtraFeatureColumns != null && i < args.ExtraFeatureColumns.Length; i++) [](start = 12, length = 93)
could this be a simple foreach loop instead?
@wschinwschinJan 23, 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 guess it can't. The array size is pre-defined and I prefer not to create an intermediate list just for calling Add.
In reply to: 250316802 [](ancestors = 250316802)
@wschin
wschin
commented
Jan 23, 2019

Copy link


Copy Markdown

ContributorAuthor
 public readonly SchemaShape.Column[] FeatureColumns;

It's not related to Options. It's a state of a trainer which can be specified by multiple (I guess two) ways.
In reply to: 456913047 [](ancestors = 456913047)
Refers to: src/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs:93 in e454a19. [](commit_id = e454a19, deletion_comment = False)
shmoradims
shmoradims
approved these changes
Jan 23, 2019
View reviewed changes
@shmoradimsshmoradims
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

:shipit:
@wschin
wschin
merged commit 053062b
into
dotnet:masterJan 23, 2019
@wschin
wschin
deleted the

adv-ffm-args

branch
January 23, 2019 18:38
@abgoswamabgoswam
mentioned this pull request
Jan 28, 2019

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 25, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@sfilipisfilipiAwaiting requested review from sfilipi

2 more reviewers

@abgoswamabgoswamabgoswam approved these changes
@shmoradimsshmoradimsshmoradims approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@wschinwschin

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@wschin@abgoswam@shmoradims

, '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
Multiple feature columns in FFM - #2205
Merged
wschin merged 5 commits into
dotnet:masterfrom 
wschin:adv-ffm-args
Jan 23, 2019
Merged
Multiple feature columns in FFM#2205
wschin merged 5 commits into
dotnet:masterfrom 
wschin:adv-ffm-args
Conversation
@wschin
@wschinwschin
commented
Jan 22, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR somehow provides a solution to #2179 regarding FFM via allowing multiple feature column names in advanced trainer arguments.
Strategy: adding one extra field to Arguments of FFM; that filed is
/// <summary>/// Extra feature column names. The column named <see cref="LearnerInputBase.FeatureColumn"/> stores features from the first field./// The i-th string in <see cref="ExtraFeatureColumns"/> stores the name of the (i+1)-th field's feature column./// </summary>[Argument(ArgumentType.Multiple,HelpText="Extra columns to use for feature vectors. The i-th specified string denotes the column containing features form the (i+1)-th field."+" Note that the first field is specified by \"feat\" instead of \"exfeat\".",ShortName="exfeat",SortOrder=7)]publicstring[]ExtraFeatureColumns;
@wschinwschin
self-assigned this
Jan 22, 2019
abgoswam
abgoswam
reviewed
Jan 22, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.SamplesUtils/SamplesDatasetUtils.cs
 Field1 = new float[_simpleBinaryClassSampleFeatureLength],
 Field2 = new float[_simpleBinaryClassSampleFeatureLength] };
 // Fill feature vector according the assigned label.
 for (int j = 0; j < 10; ++j)
@abgoswamabgoswamJan 22, 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.

10 [](start = 36, length = 2)
_simpleBinaryClassSampleFeatureLength ? #Closed
@wschinwschinJan 22, 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.

Nice catch. Fixed.
In reply to: 249993941 [](ancestors = 249993941)
abgoswam
abgoswam
reviewed
Jan 22, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.SamplesUtils/SamplesDatasetUtils.cs
 {
 public bool Label;

 [VectorType(_simpleBinaryClassSampleFeatureLength)]
@abgoswamabgoswamJan 22, 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.

VectorType [](start = 13, length = 10)
am curious - is this attribute required ? #Closed
@wschinwschinJan 22, 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.

It is required.
In reply to: 249994977 [](ancestors = 249994977)
abgoswam
abgoswam
previously approved these changes
Jan 22, 2019
View reviewed changes
@abgoswamabgoswam
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:
wschin
wschin
commented
Jan 23, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.FastTree/TreeEnsemble/TreeEnsemble.cs
Outdated

@abgoswam
abgoswam
dismissed
their stale reviewJanuary 23, 2019 18:01
revoking review
abgoswam
abgoswam
reviewed
Jan 23, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/TrainerEstimators/FAFMEstimator.cs

 // Customized the field names.
 ffmArgs.FeatureColumn = nameof(DatasetUtils.FfmExample.Field0); // First field.
 ffmArgs.ExtraFeatureColumns = new[]{ nameof(DatasetUtils.FfmExample.Field1), nameof(DatasetUtils.FfmExample.Field2) };
@abgoswamabgoswamJan 23, 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.

ExtraFeatureColumns [](start = 20, length = 19)
this looks slightly odd . isn't it ?
am curious -- why move away from the convention used in iteration #2, where we were re-defining FeatureColumn as a string[]
@wschinwschinJan 23, 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.

Framework of generating entry points generates the hidden field (old feature column name) and therefore we have two fields with the same name and an error.
In reply to: 250309618 [](ancestors = 250309618)
@abgoswam
abgoswam
commented
Jan 23, 2019

Copy link


Copy Markdown

Member
 public readonly SchemaShape.Column[] FeatureColumns;

internal.
We do not want this exposed especially since it holds content from Options.FeatureColumn and Options.ExtraFeatureColumns
Refers to: src/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs:93 in e454a19. [](commit_id = e454a19, deletion_comment = False)
abgoswam
abgoswam
approved these changes
Jan 23, 2019
View reviewed changes
@abgoswamabgoswam
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:
shmoradims
shmoradims
reviewed
Jan 23, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs
 FeatureColumns[0] = new SchemaShape.Column(args.FeatureColumn, SchemaShape.Column.VectorKind.Vector, NumberType.R4, false);

 // Add 2nd, 3rd, and other fields from a FFM-specific argument, args.ExtraFeatureColumns.
 for (int i = 0; args.ExtraFeatureColumns != null && i < args.ExtraFeatureColumns.Length; i++)
@shmoradimsshmoradimsJan 23, 2019

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

for (int i = 0; args.ExtraFeatureColumns != null && i < args.ExtraFeatureColumns.Length; i++) [](start = 12, length = 93)
could this be a simple foreach loop instead?
@wschinwschinJan 23, 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 guess it can't. The array size is pre-defined and I prefer not to create an intermediate list just for calling Add.
In reply to: 250316802 [](ancestors = 250316802)
@wschin
wschin
commented
Jan 23, 2019

Copy link


Copy Markdown

ContributorAuthor
 public readonly SchemaShape.Column[] FeatureColumns;

It's not related to Options. It's a state of a trainer which can be specified by multiple (I guess two) ways.
In reply to: 456913047 [](ancestors = 456913047)
Refers to: src/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs:93 in e454a19. [](commit_id = e454a19, deletion_comment = False)
shmoradims
shmoradims
approved these changes
Jan 23, 2019
View reviewed changes
@shmoradimsshmoradims
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

:shipit:
@wschin
wschin
merged commit 053062b
into
dotnet:masterJan 23, 2019
@wschin
wschin
deleted the

adv-ffm-args

branch
January 23, 2019 18:38
@abgoswamabgoswam
mentioned this pull request
Jan 28, 2019

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 25, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@sfilipisfilipiAwaiting requested review from sfilipi

2 more reviewers

@abgoswamabgoswamabgoswam approved these changes
@shmoradimsshmoradimsshmoradims approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@wschinwschin

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@wschin@abgoswam@shmoradims

, '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
Multiple feature columns in FFM - #2205
Merged
wschin merged 5 commits into
dotnet:masterfrom 
wschin:adv-ffm-args
Jan 23, 2019
Merged
Multiple feature columns in FFM#2205
wschin merged 5 commits into
dotnet:masterfrom 
wschin:adv-ffm-args
Conversation
@wschin
@wschinwschin
commented
Jan 22, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR somehow provides a solution to #2179 regarding FFM via allowing multiple feature column names in advanced trainer arguments.
Strategy: adding one extra field to Arguments of FFM; that filed is
/// <summary>/// Extra feature column names. The column named <see cref="LearnerInputBase.FeatureColumn"/> stores features from the first field./// The i-th string in <see cref="ExtraFeatureColumns"/> stores the name of the (i+1)-th field's feature column./// </summary>[Argument(ArgumentType.Multiple,HelpText="Extra columns to use for feature vectors. The i-th specified string denotes the column containing features form the (i+1)-th field."+" Note that the first field is specified by \"feat\" instead of \"exfeat\".",ShortName="exfeat",SortOrder=7)]publicstring[]ExtraFeatureColumns;
@wschinwschin
self-assigned this
Jan 22, 2019
abgoswam
abgoswam
reviewed
Jan 22, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.SamplesUtils/SamplesDatasetUtils.cs
 Field1 = new float[_simpleBinaryClassSampleFeatureLength],
 Field2 = new float[_simpleBinaryClassSampleFeatureLength] };
 // Fill feature vector according the assigned label.
 for (int j = 0; j < 10; ++j)
@abgoswamabgoswamJan 22, 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.

10 [](start = 36, length = 2)
_simpleBinaryClassSampleFeatureLength ? #Closed
@wschinwschinJan 22, 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.

Nice catch. Fixed.
In reply to: 249993941 [](ancestors = 249993941)
abgoswam
abgoswam
reviewed
Jan 22, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.SamplesUtils/SamplesDatasetUtils.cs
 {
 public bool Label;

 [VectorType(_simpleBinaryClassSampleFeatureLength)]
@abgoswamabgoswamJan 22, 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.

VectorType [](start = 13, length = 10)
am curious - is this attribute required ? #Closed
@wschinwschinJan 22, 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.

It is required.
In reply to: 249994977 [](ancestors = 249994977)
abgoswam
abgoswam
previously approved these changes
Jan 22, 2019
View reviewed changes
@abgoswamabgoswam
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:
wschin
wschin
commented
Jan 23, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.FastTree/TreeEnsemble/TreeEnsemble.cs
Outdated

@abgoswam
abgoswam
dismissed
their stale reviewJanuary 23, 2019 18:01
revoking review
abgoswam
abgoswam
reviewed
Jan 23, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/TrainerEstimators/FAFMEstimator.cs

 // Customized the field names.
 ffmArgs.FeatureColumn = nameof(DatasetUtils.FfmExample.Field0); // First field.
 ffmArgs.ExtraFeatureColumns = new[]{ nameof(DatasetUtils.FfmExample.Field1), nameof(DatasetUtils.FfmExample.Field2) };
@abgoswamabgoswamJan 23, 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.

ExtraFeatureColumns [](start = 20, length = 19)
this looks slightly odd . isn't it ?
am curious -- why move away from the convention used in iteration #2, where we were re-defining FeatureColumn as a string[]
@wschinwschinJan 23, 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.

Framework of generating entry points generates the hidden field (old feature column name) and therefore we have two fields with the same name and an error.
In reply to: 250309618 [](ancestors = 250309618)
@abgoswam
abgoswam
commented
Jan 23, 2019

Copy link


Copy Markdown

Member
 public readonly SchemaShape.Column[] FeatureColumns;

internal.
We do not want this exposed especially since it holds content from Options.FeatureColumn and Options.ExtraFeatureColumns
Refers to: src/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs:93 in e454a19. [](commit_id = e454a19, deletion_comment = False)
abgoswam
abgoswam
approved these changes
Jan 23, 2019
View reviewed changes
@abgoswamabgoswam
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:
shmoradims
shmoradims
reviewed
Jan 23, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs
 FeatureColumns[0] = new SchemaShape.Column(args.FeatureColumn, SchemaShape.Column.VectorKind.Vector, NumberType.R4, false);

 // Add 2nd, 3rd, and other fields from a FFM-specific argument, args.ExtraFeatureColumns.
 for (int i = 0; args.ExtraFeatureColumns != null && i < args.ExtraFeatureColumns.Length; i++)
@shmoradimsshmoradimsJan 23, 2019

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

for (int i = 0; args.ExtraFeatureColumns != null && i < args.ExtraFeatureColumns.Length; i++) [](start = 12, length = 93)
could this be a simple foreach loop instead?
@wschinwschinJan 23, 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 guess it can't. The array size is pre-defined and I prefer not to create an intermediate list just for calling Add.
In reply to: 250316802 [](ancestors = 250316802)
@wschin
wschin
commented
Jan 23, 2019

Copy link


Copy Markdown

ContributorAuthor
 public readonly SchemaShape.Column[] FeatureColumns;

It's not related to Options. It's a state of a trainer which can be specified by multiple (I guess two) ways.
In reply to: 456913047 [](ancestors = 456913047)
Refers to: src/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs:93 in e454a19. [](commit_id = e454a19, deletion_comment = False)
shmoradims
shmoradims
approved these changes
Jan 23, 2019
View reviewed changes
@shmoradimsshmoradims
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

:shipit:
@wschin
wschin
merged commit 053062b
into
dotnet:masterJan 23, 2019
@wschin
wschin
deleted the

adv-ffm-args

branch
January 23, 2019 18:38
@abgoswamabgoswam
mentioned this pull request
Jan 28, 2019

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 25, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@sfilipisfilipiAwaiting requested review from sfilipi

2 more reviewers

@abgoswamabgoswamabgoswam approved these changes
@shmoradimsshmoradimsshmoradims approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@wschinwschin

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@wschin@abgoswam@shmoradims

, '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
Multiple feature columns in FFM - #2205
Merged
wschin merged 5 commits into
dotnet:masterfrom 
wschin:adv-ffm-args
Jan 23, 2019
Merged
Multiple feature columns in FFM#2205
wschin merged 5 commits into
dotnet:masterfrom 
wschin:adv-ffm-args
Conversation
@wschin
@wschinwschin
commented
Jan 22, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR somehow provides a solution to #2179 regarding FFM via allowing multiple feature column names in advanced trainer arguments.
Strategy: adding one extra field to Arguments of FFM; that filed is
/// <summary>/// Extra feature column names. The column named <see cref="LearnerInputBase.FeatureColumn"/> stores features from the first field./// The i-th string in <see cref="ExtraFeatureColumns"/> stores the name of the (i+1)-th field's feature column./// </summary>[Argument(ArgumentType.Multiple,HelpText="Extra columns to use for feature vectors. The i-th specified string denotes the column containing features form the (i+1)-th field."+" Note that the first field is specified by \"feat\" instead of \"exfeat\".",ShortName="exfeat",SortOrder=7)]publicstring[]ExtraFeatureColumns;
@wschinwschin
self-assigned this
Jan 22, 2019
abgoswam
abgoswam
reviewed
Jan 22, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.SamplesUtils/SamplesDatasetUtils.cs
 Field1 = new float[_simpleBinaryClassSampleFeatureLength],
 Field2 = new float[_simpleBinaryClassSampleFeatureLength] };
 // Fill feature vector according the assigned label.
 for (int j = 0; j < 10; ++j)
@abgoswamabgoswamJan 22, 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.

10 [](start = 36, length = 2)
_simpleBinaryClassSampleFeatureLength ? #Closed
@wschinwschinJan 22, 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.

Nice catch. Fixed.
In reply to: 249993941 [](ancestors = 249993941)
abgoswam
abgoswam
reviewed
Jan 22, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.SamplesUtils/SamplesDatasetUtils.cs
 {
 public bool Label;

 [VectorType(_simpleBinaryClassSampleFeatureLength)]
@abgoswamabgoswamJan 22, 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.

VectorType [](start = 13, length = 10)
am curious - is this attribute required ? #Closed
@wschinwschinJan 22, 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.

It is required.
In reply to: 249994977 [](ancestors = 249994977)
abgoswam
abgoswam
previously approved these changes
Jan 22, 2019
View reviewed changes
@abgoswamabgoswam
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:
wschin
wschin
commented
Jan 23, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.FastTree/TreeEnsemble/TreeEnsemble.cs
Outdated

@abgoswam
abgoswam
dismissed
their stale reviewJanuary 23, 2019 18:01
revoking review
abgoswam
abgoswam
reviewed
Jan 23, 2019
View reviewed changes
Comment threadtest/Microsoft.ML.Tests/TrainerEstimators/FAFMEstimator.cs

 // Customized the field names.
 ffmArgs.FeatureColumn = nameof(DatasetUtils.FfmExample.Field0); // First field.
 ffmArgs.ExtraFeatureColumns = new[]{ nameof(DatasetUtils.FfmExample.Field1), nameof(DatasetUtils.FfmExample.Field2) };
@abgoswamabgoswamJan 23, 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.

ExtraFeatureColumns [](start = 20, length = 19)
this looks slightly odd . isn't it ?
am curious -- why move away from the convention used in iteration #2, where we were re-defining FeatureColumn as a string[]
@wschinwschinJan 23, 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.

Framework of generating entry points generates the hidden field (old feature column name) and therefore we have two fields with the same name and an error.
In reply to: 250309618 [](ancestors = 250309618)
@abgoswam
abgoswam
commented
Jan 23, 2019

Copy link


Copy Markdown

Member
 public readonly SchemaShape.Column[] FeatureColumns;

internal.
We do not want this exposed especially since it holds content from Options.FeatureColumn and Options.ExtraFeatureColumns
Refers to: src/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs:93 in e454a19. [](commit_id = e454a19, deletion_comment = False)
abgoswam
abgoswam
approved these changes
Jan 23, 2019
View reviewed changes
@abgoswamabgoswam
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:
shmoradims
shmoradims
reviewed
Jan 23, 2019
View reviewed changes
Comment threadsrc/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs
 FeatureColumns[0] = new SchemaShape.Column(args.FeatureColumn, SchemaShape.Column.VectorKind.Vector, NumberType.R4, false);

 // Add 2nd, 3rd, and other fields from a FFM-specific argument, args.ExtraFeatureColumns.
 for (int i = 0; args.ExtraFeatureColumns != null && i < args.ExtraFeatureColumns.Length; i++)
@shmoradimsshmoradimsJan 23, 2019

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

for (int i = 0; args.ExtraFeatureColumns != null && i < args.ExtraFeatureColumns.Length; i++) [](start = 12, length = 93)
could this be a simple foreach loop instead?
@wschinwschinJan 23, 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 guess it can't. The array size is pre-defined and I prefer not to create an intermediate list just for calling Add.
In reply to: 250316802 [](ancestors = 250316802)
@wschin
wschin
commented
Jan 23, 2019

Copy link


Copy Markdown

ContributorAuthor
 public readonly SchemaShape.Column[] FeatureColumns;

It's not related to Options. It's a state of a trainer which can be specified by multiple (I guess two) ways.
In reply to: 456913047 [](ancestors = 456913047)
Refers to: src/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs:93 in e454a19. [](commit_id = e454a19, deletion_comment = False)
shmoradims
shmoradims
approved these changes
Jan 23, 2019
View reviewed changes
@shmoradimsshmoradims
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

:shipit:
@wschin
wschin
merged commit 053062b
into
dotnet:masterJan 23, 2019
@wschin
wschin
deleted the

adv-ffm-args

branch
January 23, 2019 18:38
@abgoswamabgoswam
mentioned this pull request
Jan 28, 2019

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Mar 25, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@sfilipisfilipiAwaiting requested review from sfilipi

2 more reviewers

@abgoswamabgoswamabgoswam approved these changes
@shmoradimsshmoradimsshmoradims approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@wschinwschin

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@wschin@abgoswam@shmoradims