-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix column purpose for PipelineSweeperMacro #461
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 8 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
e1fa741
Merge pull request #1 from dotnet/master
abgoswam 703cb38
Merge pull request #2 from dotnet/master
abgoswam c3c2125
Merge pull request #3 from dotnet/master
abgoswam 17026a4
Adding arguments to PipelineSweep Macro
abgoswam c76181c
updating the unit tests
abgoswam 28371fe
taking care of review comments; adding validations for Label, Weight,…
abgoswam 310b395
taking care of some review comments
abgoswam 4fbaf14
some code cleanup
abgoswam 3ca4da9
addressing PR comments
abgoswam 678d727
Merge pull request #4 from dotnet/master
abgoswam 7f94a87
Merge pull request #5 from agoswamiazureml/master
abgoswam d55b1e0
API changes to use RoleMappedData
abgoswam c624415
Merge branch 'agoswami/purpose_inference' of https://github.com/agosw…
abgoswam d4f6102
taking care of review comments
abgoswam 99172e2
using pipeline.UniqueId
abgoswam fb5ca79
taking care of review comments. update ColumnPurpose 'Group' so it is…
abgoswam 6b34d71
Merge pull request #6 from dotnet/master
abgoswam 3ad0c08
Merge pull request #7 from agoswamiazureml/master
abgoswam File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,8 @@ namespace Microsoft.ML.Runtime.PipelineInference | |
| /// </summary> | ||
| public interface IPipelineOptimizer | ||
| { | ||
| PipelinePattern[] GetNextCandidates(IEnumerable<PipelinePattern> history, int numberOfCandidates); | ||
| PipelinePattern[] GetNextCandidates(IEnumerable<PipelinePattern> history, int numberOfCandidates, | ||
| Dictionary<string, ColumnPurpose> columnPurpose = null); | ||
|
|
||
| void SetSpace(TransformInference.SuggestedTransform[] availableTransforms, | ||
| RecipeInference.SuggestedRecipe.SuggestedLearner[] availableLearners, | ||
|
|
@@ -44,6 +45,7 @@ public abstract class PipelineOptimizerBase : IPipelineOptimizer | |
| protected IDataView OriginalData; | ||
| protected IDataView FullyTransformedData; | ||
| protected AutoInference.DependencyMap DependencyMapping; | ||
| protected Dictionary<string, ColumnPurpose> columnPurpose; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
C# naming conventions, please. #Resolved
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| protected readonly IHostEnvironment Env; | ||
| protected readonly IHost Host; | ||
| protected readonly Dictionary<long, bool> TransformsMaskValidity; | ||
|
|
@@ -60,7 +62,8 @@ protected PipelineOptimizerBase(IHostEnvironment env, IHost host) | |
| ProbUtils = new SweeperProbabilityUtils(host); | ||
| } | ||
|
|
||
| public abstract PipelinePattern[] GetNextCandidates(IEnumerable<PipelinePattern> history, int numberOfCandidates); | ||
| public abstract PipelinePattern[] GetNextCandidates(IEnumerable<PipelinePattern> history, int numberOfCandidates, | ||
| Dictionary<string, ColumnPurpose> columnPurpose = null); | ||
|
|
||
| public virtual void SetSpace(TransformInference.SuggestedTransform[] availableTransforms, | ||
| RecipeInference.SuggestedRecipe.SuggestedLearner[] availableLearners, | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am somewhat unenthusiastic about this, since it basically seems like role-mappings, except in reverse (which also makes it, incidentally, less capable). Is there any possibility of re-using that concept, rather than inventing another thing that acts more or less just like it? #Resolved
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems the 'recipe framework' added the concept of 'Purpose' previously. Not quite sure why the decision was made at that time to introduce Purpose as an additional concept, instead of re-using the concept of Roles and extending it.
<snip from InferenceUtil.cs>
public enum ColumnPurpose
{
Ignore = 0,
Name = 1,
Label = 2,
NumericFeature = 3,
CategoricalFeature = 4,
TextFeature = 5,
Weight = 6,
GroupId = 7,
ImagePath = 8
}
Is there a simple way to extend Roles to include purpose (e.g. 'TextFeature', 'Ignore') ? My understanding is that it would be a bigger change that warrants a separate design / PR. Let me know if that sounds reasonable.
This PR is to address the particular case where 'recipe framework' inferred the purpose incorrectly. In sich cases the user has no way of overriding the inferred purpose when using the PipelineSweeperMacro. This PR unblocks such scenarios.
In reply to: 199661532 [](ancestors = 199661532)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't extend roles, you simply use them. If you want to introduce roles appropriate for your task, that is totally fine and supported. I wonder though if the issue is that you do not have an actual
ISchemaimplementation we're working on? #ResolvedUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is ColumnPurpose and Roles the same things? Feels to me they are not. There is a ColumnPurpose of "ImagePath" but not such a Role, and @justin has ideas about adding more purposes. Purposes would be used to just communicate to Experts what transforms to suggest for the pipelines. Learners would not neccessarly need to know about them.
I have been wondering whether it makes more sense to extend the Loader columns to have a field/property of type ColumnPurpose, rather than pass yet another list of purposes, so they become part of the Schema, and whatever needs the purposes (the inference code) can just look it up in the columns.
In reply to: 200406010 [](ancestors = 200406010)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RecipeInference uses ColumnPurpose to infer the set of transforms to use (and also the columns used for each transform) .
In the GUI users can modify the ColumnPurpose through the wizard. We are adding arguments to the Macro so users can specify the purpose when using the macro programmatically.
Am not sure which
ISchemaimplementation is being referred to. If you are referring to anISchemaimplementation at the end of all transformations then yes, we do not have the finalISchemaimplemenation at this point in the computation. Hence the need to pass ColumnPurpose so we can infer the set of transforms and get to the finalISchemaimplementation.Or, is the suggestion to completely do away with the notion of ColumnPurpose ?
In reply to: 200410413 [](ancestors = 200410413,200406010)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a couple of changes to address this:
In reply to: 200723741 [](ancestors = 200723741,200410413,200406010)