Skip to content

Slightly simplified version of adding KeyToValue for onnx export #4881

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

Closed
wants to merge 1 commit into from
Closed

Slightly simplified version of adding KeyToValue for onnx export #4881

wants to merge 1 commit into from

Conversation

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

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
Copy link
Member

ganik commented Feb 24, 2020

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