Skip to content

Transformer for Concat #896

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Sep 14, 2018
Merged

Transformer for Concat #896

merged 5 commits into from
Sep 14, 2018

Conversation

Zruty0
Copy link
Contributor

@Zruty0 Zruty0 commented Sep 12, 2018

Proper conversion of ConcatTransform

@Zruty0 Zruty0 added the API Issues pertaining the friendly API label Sep 12, 2018
@Zruty0 Zruty0 self-assigned this Sep 12, 2018
@Zruty0 Zruty0 mentioned this pull request Sep 12, 2018
Closed
@Ivanidzo4ka
Copy link
Contributor

Ivanidzo4ka commented Sep 13, 2018 •
edited by Zruty0
Loading 

                viewTrain);

Don't you want to get rid of calls to Arguments class? #Resolved

Refers to: src/Microsoft.ML/Runtime/EntryPoints/FeatureCombiner.cs:79 in 0b239ea. [](commit_id = 0b239ea, deletion_comment = False)

Ivanidzo4ka
Ivanidzo4ka reviewed Sep 13, 2018
View reviewed changes
src/Microsoft.ML.Data/DataView/RowToRowMapperTransform.cs
@@ -134,15 +134,15 @@ public sealed class RowToRowMapperTransform : RowToRowTransformBase, IRowToRowMa
{
private sealed class Bindings : ColumnBindingsBase
{
private readonly RowToRowMapperTransform _parent;
private readonly IRowMapper _mapper;
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Sep 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

IRowMapper [](start = 29, length = 10)

Would this stop double mapper creation which we have right now for each RowToRowMapperTransform instantiation? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think yes

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

Ivanidzo4ka
Ivanidzo4ka reviewed Sep 13, 2018
View reviewed changes
src/Microsoft.ML.Data/Transforms/ConcatEstimator.cs
@@ -123,90 +112,6 @@ public SchemaShape GetOutputSchema(SchemaShape inputSchema)
}
}

// REVIEW: Note that the presence of this thing is a temporary measure only.
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Sep 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

// REVIEW: Note that the presence of this thing is a temporary measure only. [](start = 4, length = 76)

People spend time writing this code, and you deleting it without any remorse! #ByDesign

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extra credit for removing Tom's code!

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

Copy link
Contributor

Choose a reason for hiding this comment

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

If a code falls in the forest...

In reply to: 217551182 [](ancestors = 217551182,217230777)

Ivanidzo4ka
Ivanidzo4ka reviewed Sep 13, 2018
View reviewed changes
src/Microsoft.ML.Data/Transforms/ConcatTransform.cs Outdated
@@ -18,19 +18,33 @@
using Microsoft.ML.Runtime.Model.Onnx;
using Microsoft.ML.Runtime.Model.Pfa;
using Newtonsoft.Json.Linq;
using Microsoft.ML.Core.Data;
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Sep 13, 2018 •
edited by Zruty0
Loading

Choose a reason for hiding this comment

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

mildly shock #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed Pete, you must learn to sort. :D :D :D

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

Ivanidzo4ka
Ivanidzo4ka reviewed Sep 13, 2018
View reviewed changes
src/Microsoft.ML.Data/Transforms/ConcatTransform.cs Outdated
inputs[i].Zip(aliases[i], (name, alias) => (name, alias)));
return result;
}

private sealed class Bindings : ManyToOneColumnBindingsBase
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Sep 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Bindings [](start = 29, length = 8)

You probably forgot to delete this peace of code. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct

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

Ivanidzo4ka
Ivanidzo4ka reviewed Sep 13, 2018
View reviewed changes
src/Microsoft.ML.Data/Transforms/ConcatTransform.cs Outdated
Contracts.CheckValue(inputs, nameof(inputs));
Contracts.CheckParam(inputs.Any(), nameof(inputs), "Can not be empty");

foreach (var (name, alias) in inputs)
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Sep 13, 2018 •
edited by Zruty0
Loading

Choose a reason for hiding this comment

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

alias [](start = 36, length = 5)

Can we have a test in which we use these aliases, or maybe some kind of ///summary on top of this constructor with description of aliases.
Right now it's mystery for me, and any potential user. #Resolved

@TomFinley
Copy link
Contributor

TomFinley commented Sep 13, 2018 •
edited
Loading 

                viewTrain);

Internally it is fine for now. Externally, yes I agree, but this is not relevant.

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

Refers to: src/Microsoft.ML/Runtime/EntryPoints/FeatureCombiner.cs:79 in 0b239ea. [](commit_id = 0b239ea, deletion_comment = False)

@Zruty0
Copy link
Contributor Author

Zruty0 commented Sep 13, 2018 

                viewTrain);

yes

In reply to: 420868137 [](ancestors = 420868137,420842898)

Refers to: src/Microsoft.ML/Runtime/EntryPoints/FeatureCombiner.cs:79 in 0b239ea. [](commit_id = 0b239ea, deletion_comment = False)

TomFinley
TomFinley approved these changes Sep 14, 2018
View reviewed changes
Copy link
Contributor

@TomFinley TomFinley left a comment

Choose a reason for hiding this comment

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

Thanks @Zruty0

Ivanidzo4ka
Ivanidzo4ka approved these changes Sep 14, 2018
View reviewed changes
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka left a comment

Choose a reason for hiding this comment

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

:shipit:

@Zruty0 Zruty0 merged commit 4cb7dd9 into dotnet:master Sep 14, 2018
@Zruty0 Zruty0 deleted the feature/concat branch September 14, 2018 23:41
@sfilipi sfilipi mentioned this pull request Oct 10, 2018
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Mar 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Ivanidzo4ka Ivanidzo4ka Ivanidzo4ka approved these changes

@TomFinley TomFinley TomFinley approved these changes

Assignees

@Zruty0 Zruty0

Labels
API Issues pertaining the friendly API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Zruty0 @Ivanidzo4ka @TomFinley