Skip to content

Connect Inputs and Outputs Properly and Export CopyColumnTransform to (unofficial) ONNX operator#952

Merged
wschin merged 15 commits intodotnet:masterfrom
wschin:savecopy
Oct 10, 2018
Merged

Connect Inputs and Outputs Properly and Export CopyColumnTransform to (unofficial) ONNX operator#952
wschin merged 15 commits intodotnet:masterfrom
wschin:savecopy

Conversation

@wschin
Copy link
Copy Markdown
Contributor

@wschin wschin commented Sep 19, 2018

Related issue: #955 , #968.

  1. Add exporter for CopyColumnTransform. It only creates a ONNX node with a type field for encoding the transform's C# name.
  2. Properly connect inputs and outputs especially when they have the same name in pipeline.

1. Add exporter for CopyColumnTransform
2. Remove duplicate definitions in ONNX graph
@wschin wschin changed the title Export CopyColumnTransform to (unofficial) ONNX operator Connect Inputs and Outputs Properly and Export CopyColumnTransform to (unofficial) ONNX operator Sep 21, 2018
@markusweimer
Copy link
Copy Markdown

markusweimer commented Sep 21, 2018

This PR does not reference an issue. Can you please file one and reference it in the PR description? #Resolved

@wschin
Copy link
Copy Markdown
Contributor Author

wschin commented Sep 25, 2018

Note that the newly generated test models generate the same values as the existing ones. Experiments were run with the latest runtime. #Resolved

@codemzs codemzs requested a review from TomFinley October 2, 2018 16:27
/// </summary>
/// <param name="variableName">examined string</param>
/// <returns>True if the input argument has been used to denote an ONNX variable. Otherwise, False.</returns>
public abstract bool IsDefined(string variableName);
Copy link
Copy Markdown
Member

@codemzs codemzs Oct 2, 2018

Choose a reason for hiding this comment

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

IsDefined [](start = 29, length = 9)

Can we please rename this to be more specific like "IsVariableDefined"? #Resolved

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure.


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

Comment thread src/Microsoft.ML.Onnx/SaveOnnxCommand.cs
Copy link
Copy Markdown
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
Copy Markdown
Contributor

@shauheen shauheen 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
Copy Markdown
Member

@vaeksare vaeksare left a comment

Choose a reason for hiding this comment

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

:shipit:


public void SaveAsOnnx(OnnxContext ctx)
{
var infos = GetOutputColumns();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You are not using this infos anywhere. I am guessing it was originally meant to be used in the foreach but you decided to use _columns instead?

@wschin wschin merged commit 55f6c22 into dotnet:master Oct 10, 2018
@wschin wschin deleted the savecopy branch October 10, 2018 05:43
@ghost ghost locked as resolved and limited conversation to collaborators Mar 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants