Skip to content

Conversation

@melanciani
Copy link
Contributor

  • Fix minor typos in convert method and supported functions.
  • No issue is referenced as specified in the guidelines

@melanciani
Copy link
Contributor Author

I'm however not sure about the ONNX parameter sentence : The *onnx* backend requires a test_input of the initial types, set through the extra_config parameter. Shouldn't the set through the extra_config parameter be removed ?

@ksaur
Copy link
Contributor

ksaur commented Aug 31, 2022

Hi @RomanBredehoft Thanks for catching these!

I'm however not sure about the ONNX parameter sentence : The *onnx* backend requires a test_input of the initial types, set through the extra_config parameter. Shouldn't the set through the extra_config parameter be removed ?

You're right, this is not set through extra_config. I know that things work best with ONNX when the user provides initial input (ex: convert(onnx_ml_model, "onnx", Xtest)), but if they don't we try to generate it...not always successfully!

I think we should change this to something like:
The *onnx* backend works best with a test_input, but we try to generate one if none is provided. Thoughts @interesaaat ?

@interesaaat
Copy link
Collaborator

Hi @RomanBredehoft Thanks for catching these!

I'm however not sure about the ONNX parameter sentence : The *onnx* backend requires a test_input of the initial types, set through the extra_config parameter. Shouldn't the set through the extra_config parameter be removed ?

You're right, this is not set through extra_config. I know that things work best with ONNX when the user provides initial input (ex: convert(onnx_ml_model, "onnx", Xtest)), but if they don't we try to generate it...not always successfully!

I think we should change this to something like: The *onnx* backend works best with a test_input, but we try to generate one if none is provided. Thoughts @interesaaat ?

Yes, agreed.

@ksaur
Copy link
Contributor

ksaur commented Sep 10, 2022

Hi @RomanBredehoft , can you make that change on line 414? (The *onnx* backend works best with a test_input, but we try to generate one if none is provided.)

@melanciani
Copy link
Contributor Author

melanciani commented Sep 12, 2022

@ksaur done ! just rebased and made the change

@interesaaat
Copy link
Collaborator

Thanks for the contribution!

@interesaaat interesaaat merged commit a090681 into microsoft:main Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants