Skip to content
This repository has been archived by the owner on Dec 21, 2023. It is now read-only.

Image Classification Evaluation #1335

Merged
merged 24 commits into from
Jan 30, 2019

Conversation

abhishekpratapa
Copy link
Collaborator

  • added support for image classification evaluation in TuriCreate

@nickjong nickjong added this to the 5.3 milestone Jan 23, 2019
}else if(this.props.type == "percent"){
return (
<div className="TCEvaluationConfusionCell">

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the content in the percent empty?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we don't actually have a percent requirement yet in the Confusions Evaluation Table, so I left it unimplemented.

}

sortData = (a, b) => {
if(this.props.sort_by_confusions == "actual" && !this.props.sort_direction_confusions){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this function can be simplified into:

const {sort_by_confusions, sort_direction_confusions} = this.props;
if (sort_by_confusions) {
  if (sort_direction_confusions) {
	return a[sort_by_confusions] < b[sort_by_confusions] ? -1 : 1;
  } else {
    return a[sort_by_confusions] > b[sort_by_confusions] ? -1 : 1;
  }
}

I also wonder if the currently code handles ties consistently. Shouldn't equal return zero?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yaaa, it should prob return zero, however, I'm not sure if it changes the sort that much. Maybe it does?

@znation
Copy link
Contributor

znation commented Jan 28, 2019

@abhishekpratapa It looks like you're getting a test failure:

self = <turicreate.test.test_logistic_classifier.LogisticRegressionClassifierModelTest_binary_classification_integer_target testMethod=test__list_fields>
msg = "Items in the first set but not the second:\n'training_predictions'"

However, I think the test should simply be updated to accommodate the new field. Seems that training_predictions is added intentionally in this PR, and shouldn't break existing code. What we should consider is:

  • What will a serialized model, saved prior to this PR, return when this field is called?
  • What will a serialized model, saved after this PR, return when this field is called? (i.e., are we persisting training predictions as part of the model artifact?) If we are persisting to disk, does this massively inflate the model size?
  • In either case, if we don't get valid training predictions, is our new evaluation UX broken without it? What should the UX be in that case?

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.

5 participants