Skip to content

Slightly simplified version of adding KeyToValue for onnx export - #4881

Closed
harishsk wants to merge 1 commit into
dotnet:masterfrom
harishsk:nimbusKeyToValue
Closed

Slightly simplified version of adding KeyToValue for onnx export#4881
harishsk wants to merge 1 commit into
dotnet:masterfrom
harishsk:nimbusKeyToValue

Conversation

@harishsk

Copy link
Copy Markdown
Contributor

This is a variation of @antoniovs1029 's fix. This adds KeyToValueMappingTransformer in only one place. The results are identical between the two.

@harishsk
harishsk requested a review from a team as a code owner February 24, 2020 06:28
@antoniovs1029

Copy link
Copy Markdown
Contributor

I think the only difference between this and my fix is that in here all of the non-hidden columns at the output schema, which are KeyDataViewTypes, will be mapped back to values.

In my other PR this doesn't happen for all columns, only for PredictedLabel and for columns that had the same name and where KeyDataViewTypes both in the input and output.

A scenario were my PR and this PR would be different, is one where the user actually wants to apply a ToKey in his NimbusML pipeline, and actually expects to see only keys in his output (from ONNX or otherwise). In this scenario I believe your PR here would automatically add a KeyToValueTransformer for that column, whereas my PR wouldn't, since the user actually wants to see keys. Gani was the one who mentioned this scenario to me some days ago, not sure if it's still something we would want to avoid.

@ganik

ganik commented Feb 24, 2020

Copy link
Copy Markdown
Contributor

If ToKey applied on string and its output is among output columns of pipeline then Nimbus in python gives back categorical type, which is a tuple (string Value, int/uint Key). In ONNX we dont have categoricals so we can output either Value or a Key. Value is useful for presentation layer, like doing print etc. If there is a downstream learner that consumes ToKey output or we to stitch 2 ONNX models then its done off Key, while Value is ignored. I would sacrifice presentational purposes for functional ones, and would return Key instead of Value in ONNX models. PredictedLabel is the only exception as its usually final output and (ignoring stacking) used for presentation purposes only.

@harishsk

Copy link
Copy Markdown
Contributor Author

This PR is superseded by #4841 . Closing.

@harishsk harishsk closed this Feb 25, 2020
@harishsk
harishsk deleted the nimbusKeyToValue 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.

3 participants