Skip to content

Added multiple related fixes to enable automatic addition of KeyToValue - #4878

Merged
harishsk merged 2 commits into
dotnet:masterfrom
harishsk:onnxMultipleFixes
Feb 24, 2020
Merged

Added multiple related fixes to enable automatic addition of KeyToValue#4878
harishsk merged 2 commits into
dotnet:masterfrom
harishsk:onnxMultipleFixes

Conversation

@harishsk

Copy link
Copy Markdown
Contributor

This PR includes a number of fixes to enable automatic addition of KeyToValue in the Nimbus codepath. Specifically:

  • Fixed BinaryClassfierScorer to support exporting key types
  • Fixed PredictedLabelScorerBase to include the right types and shapes in the onnx graph
  • Fixed OneVersusAllTrainer to include shape information
  • Fixed SaveAsOnnxCommand to include type information in the last Identity node
  • Fixed OptionalColumnTransform to have the correct variable names and types. (This one is a preemptive fix. It fixes a crash that happens with the code in the ORT master branch)

@harishsk
harishsk requested a review from a team as a code owner February 22, 2020 10:04
OnnxNode node;
var binarizerOutput = ctx.AddIntermediateVariable(null, "BinarizerOutput", true);
string opType = "Binarizer";
var binarizerOutput = ctx.AddIntermediateVariable(NumberDataViewType.Single, "BinarizerOutput", false);

@ganik ganik Feb 24, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

false [](start = 108, length = 5)

What does this false mean?
#Resolved

@harishsk harishsk Feb 24, 2020

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.

False = Do not skip adding shape and type information (or in other words, add shape and type information). False is the default value. Technically it is not necessary to specify it.


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

{
Host.Assert(Bindings.InfoCount >= 3);
scoreColumn = outColumnNames[2];
var one = ctx.AddInitializer(1.0f, "one");

@ganik ganik Feb 24, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ctx.AddInitializer(1.0f, "one"); [](start = 26, length = 32)

Is this +1 ? to be 1 based? #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.

Yes, this is to make it one based and make it consistent with ML.NET results.


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

else if (info.TypeSrc.GetItemType().Equals(NumberDataViewType.Double))
{
// LabelEncoder doesn't support double tensors, so values are cast to floats
var castOutput = ctx.AddIntermediateVariable(null, "castOutput", true);

@ganik ganik Feb 24, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

null [](start = 65, length = 4)

How did it work with null before? Was there an exception? #Resolved

@harishsk harishsk Feb 24, 2020

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.

If the last parameter is true, it skips adding shape and type information and therefore accepts null. This still works, but I am prepping some parts of the code base for issues I have seen when run against the master branch of ORT.
There will be an exception if you specify null and set the last parameter to false.


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

@ganik ganik left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:shipit:

@harishsk
harishsk merged commit a0997c6 into dotnet:master Feb 24, 2020
@harishsk
harishsk deleted the onnxMultipleFixes branch April 21, 2020 23:58
@ghost ghost locked as resolved and limited conversation to collaborators Mar 19, 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.

2 participants