Skip to content

Issues with ImageClassificationTransformer #4276

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
yaeldekel opened this issue Oct 2, 2019 · 1 comment · Fixed by #4372
Closed

Issues with ImageClassificationTransformer #4276

yaeldekel opened this issue Oct 2, 2019 · 1 comment · Fixed by #4372
Assignees

Comments

@yaeldekel
Copy link

  • The Options class contains two string array fields: one for input column names and one for output column names. However, it always uses exactly two input columns: a label column and a features column, and it always outputs exactly two output columns: score and predicted label. This discrepancy should be addressed.
  • The transformer has a field called _labelColumnName that is serialized and deserialized and otherwise does nothing. Since after deserialization we cannot rely on having data that has a label column anyway, this field should be removed.
  • GetOutputSchema() in the estimator does not check that the type of the label column is correct (what other types, if any, should be allowed other than key type?), and neither does the constructor (it doesn't check the type of the features column either).
  • I think the constructor of the transformer should not have a boolean flag to indicate whether the transformer is being instantiated from file or from the estimator. Instead, the estimator should do the required training (including figuring out the class count), and there should be one constructor for the transformer, that is called both from the estimator's Fit() method, and from the transformer's Create(ModelLoadContext) method.
  • The estimator has a _transformer field that is not needed.
  • The transformer ctor has an unused argument: batchSize - don't know if the error is in not using it or in not needing it.
@codemzs
Copy link
Member

codemzs commented Oct 2, 2019

These are good to be addresses but not critical. I’ll consider this as a P3 and resolve them right before GA since this API undergoes changes every release to make it better.

@codemzs codemzs self-assigned this Oct 2, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 20, 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 a pull request may close this issue.

2 participants