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

ApplyOnnxModel sample #3349

Merged
merged 4 commits into from
Apr 17, 2019
Merged

ApplyOnnxModel sample #3349

merged 4 commits into from
Apr 17, 2019

Conversation

ganik
Copy link
Member

@ganik ganik commented Apr 16, 2019

part of #1209

@codecov
Copy link

codecov bot commented Apr 16, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@66ff419). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #3349   +/-   ##
=========================================
  Coverage          ?    72.7%           
=========================================
  Files             ?      807           
  Lines             ?   145172           
  Branches          ?    16225           
=========================================
  Hits              ?   105542           
  Misses            ?    35217           
  Partials          ?     4413
Flag Coverage Δ
#Debug 72.7% <ø> (?)
#production 68.23% <ø> (?)
#test 88.98% <ø> (?)

{
/// <summary>
/// Example use of OnnxEstimator in an ML.NET pipeline
/// Example use of ApplyOnnxModel in an ML.NET pipeline
/// </summary>
Copy link
Contributor

@shmoradims shmoradims Apr 16, 2019

Choose a reason for hiding this comment

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

let's delete this

(it's stating the obvious and we don't have it for other samples) #Resolved

@@ -18,29 +18,17 @@ public static void Example()
// Microsoft.ML.Onnx.TestModels nuget.
var modelPath = @"squeezenet\00000001\model.onnx";
Copy link
Contributor

@shmoradims shmoradims Apr 16, 2019

Choose a reason for hiding this comment

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

Please verify that this statement is true (you can double check with Jignesh):

  1. verify github link: Is the model.onnx in the squeezenet github link the same as squeezenet\00000001\model.onnx? I see manifest.json and 00000001 on our side which don't exist in the github link. Jignesh most likely has done an extra step to create squeezenet\00000001\model.onnx which users wouldn't know how to do.

  2. verify the nuget: Microsoft.ML.Onnx.TestModels was last updated 4 months ago on myget. It's also at version 0.0.4: https://dotnet.myget.org/feed/dotnet-core/package/nuget/Microsoft.ML.Onnx.TestModels

We also have this other similar nuget: https://dotnet.myget.org/feed/dotnet-core/package/nuget/Microsoft.ML.Onnx.TestModels

There has been a lot of reshuffling things. Please double check that we're naming the correct nuget package and also try it out locally to make sure it works.

If there are any nuances about using this API, please add it to it's xml documentation.

Thanks.

#Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

yep its there


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

@shmoradims
Copy link
Contributor

shmoradims commented Apr 16, 2019

    /// </summary>

we're using regular // comments instead of /// (because the examples get pasted verbatim, and no xml is parsed). please change it here and below. #Resolved


Refers to: docs/samples/Microsoft.ML.Samples/Dynamic/OnnxTransform.cs:59 in 1011013. [](commit_id = 1011013, deletion_comment = False)

/// <summary>
/// inputSize is the overall dimensions of the model input tensor.
/// </summary>
// <summary>
Copy link
Contributor

@shmoradims shmoradims Apr 17, 2019

Choose a reason for hiding this comment

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

i mean let's delete the xml tags like

and just keep the comment text:

// inputSize is the overall dimensions of the model input tensor. #Resolved

Copy link
Contributor

@shmoradims shmoradims 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
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:

@ganik ganik merged commit c449625 into dotnet:master Apr 17, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 22, 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.

3 participants