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

tidy classifiers #433

Merged
merged 5 commits into from Sep 6, 2014

Conversation

Projects
None yet
2 participants
@jabooth
Member

jabooth commented Sep 6, 2014

  1. Removed needless classifier function (it simply applied the function it was given, as classifiers are callables just like features we can just go ahead and invoke them directly)
  2. module rename: clm.classifierfunctions -> clm.classifiers
  3. classifier_type -> classifiers in CLM classes (now consistent with features from feature_type introduced in #426)
  4. Improved documentation for what classifiers concretely are (callables returning callable) rather than what they may be (simple function returning a closure, class instance with __call__ method).

Note that this is designed on top of #432, so the diff will look large until that is first brought in.

@jabooth jabooth added the in progress label Sep 6, 2014

@jabooth

This comment has been minimized.

Member

jabooth commented Sep 6, 2014

@jalabort I've just realised that I've got this slightly wrong. Let me just check I have it straight how it should be:

  1. As with AAMs there are three important objects related to CLMs - CLMBuilder, CLM, and CLMFitter.
  2. The main class a user of Menpo is expected to invoke is CLMBuilder, which will return a CLM that can be saved to disk, inspected, and most importantly used in the future to fit.
  3. An important input to CLMFitter is a callable that can be used to train a classifier. This callable (be it a closure or class with a __call__ method implemented) will be passed a matrix of data and a matrix of labels for that data. The only object we have in Menpo at the moment to do this job is called linear_svm_lr. It is expect to return a callable itself.
  4. The result of the classifier trainer above is passed into the CLM constructor for us by the CLMBuilder. This callable in turn will be passed new samples and is expected to return a classification.

The problem in this PR is I have called the input to CLMBuilder a classifier and then called it's result (which is passed into CLM) classifier too. I see you originally had CLM take a classifier, and I think this makes total sense. I now think my renaming of CLMBuilder's input to classifier from classifier_type makes things more confusing, but I think we can find a better name that classifier_type. Thoughts on:

  • classifier_factory
  • classifier_trainer
  • classifier_builder
  • ... ?

I think I like classifier_trainer (fits well with the trainer terminology in SDM), what do you think?

@jabooth

This comment has been minimized.

Owner

jabooth commented on 5b15fbf Sep 6, 2014

@jalabort (I've moved to classifier_trainers here just while it's still fresh in my mind, will be easier now to replace this with whatever name you want)

@jalabort

This comment has been minimized.

Member

jalabort commented Sep 6, 2014

Not sure where my reply to this went before... Anyway here it is:

Your second description is right. That's how the clm relate to classifiers. In fact this realtionship is very similar to the one that regressors and sdm models have.

I like classifier_trainer as a name as well! God pick!

@jabooth

This comment has been minimized.

Member

jabooth commented Sep 6, 2014

@jalabort great! Once you are happy with #432 we can bring this (and future improvements) in then, if no one else has any objections.

@jalabort

This comment has been minimized.

Member

jalabort commented Sep 6, 2014

OK, bring it in then ;-)

@jalabort

This comment has been minimized.

Member

jalabort commented Sep 6, 2014

+1

jabooth added a commit that referenced this pull request Sep 6, 2014

@jabooth jabooth merged commit 3a028bb into menpo:master Sep 6, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@jabooth jabooth deleted the jabooth:classifier_tidy branch Sep 6, 2014

@jabooth jabooth removed the in progress label Sep 6, 2014

@jalabort

This comment has been minimized.

Member

jalabort commented Sep 6, 2014

Hi James,

Your second description is right. That's how the clm relate to classifiers.

I like classifier trainer as well.

Thanks,

Joan

On 6 Sep 2014 09:47, James Booth notifications@github.com wrote:

@jalaborthttps://github.com/jalabort I've just realised that I've got this slightly wrong. Let me just check I have it straight how it should be:

  1. As with AAMs there are three important objects related to CLMs - CLMBuilder, CLM, and CLMFitter.
  2. The main class a user of Menpo is expected to invoke is CLMBuilder, which will return a CLM that can be saved to disk, inspected, and most importantly used in the future to fit.
  3. An important input to CLMFitter is a callable that can be used to train a classifier. This callable (be it a closure or class with a call method implemented) will be passed a matrix of data and a matrix of labels for that data. The only object we have in Menpo at the moment to do this job is called linear_svm_lr. It is expect to return a callable itself.
  4. The result of the classifier trainer above is passed into the CLM constructor for us by the CLMBuilder. This callable in turn will be passed new samples and is expected to return a classification.

The problem in this PR is I have called the input to CLMBuilder a classifier and then called it's result (which is passed into CLM) classifier too. I see you originally had CLM take a classifier, and I think this makes total sense. I now think my renaming of CLMBuilder's input to classifier from classifier_type makes things more confusing, but I think we can find a better name that classifier_type. Thoughts on:

  • classifier_factory
  • classifier_trainer
  • classifier_builder
  • ... ?

I think I like classifier_trainer (fits well with the trainer terminology in SDM), what do you think?


Reply to this email directly or view it on GitHubhttps://github.com//pull/433#issuecomment-54706667.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment