ONNXTransform Upgrade to Enable Non-tensor Types #3881
Conversation
wschin
changed the title
src/Microsoft.ML.OnnxTransformer/Microsoft.ML.OnnxTransformer.csproj
Outdated
Show resolved
Hide resolved
| /// Its underlying type is <see cref="IEnumerable{T}"/>, where the generic type "T" is the input argument of | ||
| /// <see cref="OnnxSequenceType.OnnxSequenceType(Type)"/>. | ||
| /// </summary> | ||
| public sealed class OnnxSequenceType : StructuredDataViewType |
There was a problem hiding this comment.
Can you put top-level classes in their own file? One class per file is usually a good recommendation.
Here, the first class in the file is "internal", which makes it look like there are no public APIs being added. But the public APIs are later in the file. #Resolved
There was a problem hiding this comment.
No problem. Btw, I still put "...DataViewType" and its "...DataViewTypeAttribute" in the same file for emphasizing that they are dual of each other.
In reply to: 298374249 [](ancestors = 298374249)
|
|
||
| private class ZipMapStringOutput | ||
| { | ||
| [OnnxSequenceType(typeof(IDictionary<string, float>))] |
There was a problem hiding this comment.
How about tests for the new OnnxMap type as well? #Resolved
There was a problem hiding this comment.
Unfortunately, ONNXRuntime doesn't allow us to create Map value as input and the Map value is always used as input of ONNX operator... I can add some smoke tests for sure.
[Update] A test using custom mapping is added.
In reply to: 298374926 [](ancestors = 298374926)
| Inputs = (options.InputColumns.Count() == 0) ? Model.InputNames.ToArray() : options.InputColumns; | ||
| Outputs = (options.OutputColumns.Count() == 0) ? Model.OutputNames.ToArray() : options.OutputColumns; | ||
| Inputs = (options.InputColumns.Count() == 0) ? Model.ModelInfo.InputNames.ToArray() : options.InputColumns; | ||
| Outputs = (options.OutputColumns.Count() == 0) ? Model.ModelInfo.OutputNames.ToArray() : options.OutputColumns; |
There was a problem hiding this comment.
options.OutputColumns.Count() == 0 [](start = 23, length = 34)
Check if we have test. #Resolved
ghost
locked as resolved and limited conversation to collaborators
The current ONNXTransform only operates on tensor types. As many ONNX models (especially classifiers) produce Seq<Map<T, float>> where T can be either int or string, we should remove that limitation.
This should fix #3900.