Skip to content
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

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

Merged
merged 2 commits into from
Feb 24, 2020
Merged

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

merged 2 commits into from
Feb 24, 2020

Conversation

harishsk
Copy link
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);
Copy link
Member

@ganik ganik Feb 24, 2020

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

Copy link
Contributor Author

@harishsk harishsk Feb 24, 2020

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");
Copy link
Member

@ganik ganik Feb 24, 2020

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
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)

@@ -804,7 +804,7 @@ private bool SaveAsOnnxCore(OnnxContext ctx, int iinfo, ColInfo info, string src
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);
Copy link
Member

@ganik ganik Feb 24, 2020

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

Copy link
Contributor Author

@harishsk harishsk Feb 24, 2020

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)

Copy link
Member

@ganik ganik left a comment

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