Skip to content

Remove value tuples from public TransformCatalog APIs #2641

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 1 commit into from
Feb 20, 2019

Conversation

yaeldekel
Copy link

Fixes #2581 .

using Microsoft.Data.DataView;
using Microsoft.ML.Data;
using Microsoft.ML.Transforms;

namespace Microsoft.ML
{
public sealed class SimpleColumnInfo
Copy link
Contributor

Choose a reason for hiding this comment

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

SimpleColumnInfo [](start = 24, length = 16)

I think @rogancarr has issue where he don't like ColumnInfo name. #2554
I wonder how much he would hate this one.

SimpleMapping,SimplePair?

Copy link
Contributor

Choose a reason for hiding this comment

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

No strong opinion, I just think that we should have a conversation about naming these things :)


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

Copy link
Author

Choose a reason for hiding this comment

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

How about ColumnNamePair or something similar? I think something containing 'mapping' or 'pair' would be too general, wouldn't it?

Or I could leave it as SimpleColumnInfo and add a comment about it in the issue so we can change it when we find a good name for ColumnInfo?


In reply to: 258297894 [](ancestors = 258297894,258285683)

Copy link
Contributor

Choose a reason for hiding this comment

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

I added mention of this column to #2554.
Let's merge this one, and figure out proper naming in that issue.

Copy link
Member

@codemzs codemzs left a comment

Choose a reason for hiding this comment

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

:shipit:

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:

@yaeldekel yaeldekel merged commit b0a1701 into dotnet:master Feb 20, 2019
@yaeldekel yaeldekel deleted the novaluetuples branch February 20, 2019 22:33
@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants