adding hints for classifier predict methods to use per scoring function; addresses issue #176

[soumyadsanyal]

Fix for issue #176

Description

1. Uses the appropriate predict or predict_proba method for a classifier depending on which model scoring function is being examined via hints in the function definition.
2. Uses make_scorer to provide an interface for the scoring methods.
3. Potential TODO (not implemented in this PR): allow the caller to explicitly specify which of predict, predict_proba, fit_and_predict methods to use instead of doing the inference for it.

Related issues or pull requests

Also possibly addresses issue #192

Pull Request requirements

• [x] Added appropriate unit test functions in the ./mlxtend/*/tests directories
• [x] Ran nosetests ./mlxtend -sv and make sure that all unit tests pass
• [x] Checked the test coverage by running nosetests ./mlxtend --with-coverage
• [x] Checked for style issues by running flake8 ./mlxtend
• [x] Added a note about the modification or contribution to the ./docs/sources/CHANGELOG.md file
• [ ] Modify documentation in the appropriate location under mlxtend/docs/sources/ (optional)
• [x] Checked that the Travis-CI build passed at https://travis-ci.org/rasbt/mlxtend

[pep8speaks]

Hello @soumyadsanyal! Thanks for updating the PR.

• In the file mlxtend/plotting/learning_curves.py, following are the PEP8 issues :

Line 13:1: E302 expected 2 blank lines, found 1 Line 41:62: W291 trailing whitespace Line 45:47: W291 trailing whitespace Line 69:1: W293 blank line contains whitespace Line 75:1: W293 blank line contains whitespace Line 78:80: E501 line too long (88 > 79 characters) Line 80:80: E501 line too long (83 > 79 characters) Line 82:1: W293 blank line contains whitespace Line 86:1: W293 blank line contains whitespace Line 94:9: E303 too many blank lines (2) Line 100:1: W293 blank line contains whitespace

Comment last updated on January 13, 2018 at 17:35 Hours UTC

[soumyadsanyal]

WIP.

[coveralls]

Coverage remained the same at 93.451% when pulling c71eb25ef01574ddeca2096db31a01cadc6f9a04 on soumyadsanyal:soumyadsanyal/learning_curves_predict_proba into 861cade7ecc09512bc4dcf3a16b064608bdeff20 on rasbt:master.

[coveralls]

Coverage remained the same at 93.451% when pulling 4890b1a405aa9430c198912a3c7880a58155775b on soumyadsanyal:soumyadsanyal/learning_curves_predict_proba into 861cade7ecc09512bc4dcf3a16b064608bdeff20 on rasbt:master.

[coveralls]

Coverage remained the same at 93.451% when pulling 8485d0ea589f889ea38431461ab2f788ed6059d2 on soumyadsanyal:soumyadsanyal/learning_curves_predict_proba into 861cade7ecc09512bc4dcf3a16b064608bdeff20 on rasbt:master.

[coveralls]

Coverage remained the same at 93.451% when pulling 9e442761b4d159b52f6e44f7d942ac41181ccca3 on soumyadsanyal:soumyadsanyal/learning_curves_predict_proba into 861cade7ecc09512bc4dcf3a16b064608bdeff20 on rasbt:master.

[coveralls]

Coverage remained the same at 93.451% when pulling 363fa028d4cc75e96af7de319ce1eb6bdd42bfe4 on soumyadsanyal:soumyadsanyal/learning_curves_predict_proba into 861cade7ecc09512bc4dcf3a16b064608bdeff20 on rasbt:master.

[coveralls]

Coverage remained the same at 93.451% when pulling 695f6511c667e1bee6464f83c50307a3bafb9b7e on soumyadsanyal:soumyadsanyal/learning_curves_predict_proba into 861cade7ecc09512bc4dcf3a16b064608bdeff20 on rasbt:master.

[coveralls]

Coverage remained the same at 93.451% when pulling 695f6511c667e1bee6464f83c50307a3bafb9b7e on soumyadsanyal:soumyadsanyal/learning_curves_predict_proba into 861cade7ecc09512bc4dcf3a16b064608bdeff20 on rasbt:master.

[soumyadsanyal]

@rasbt : this is a first pass at addressing issue #176 ; flipping over to you for review. Please let me know if any comments, corrections, suggestions, etc!

[rasbt]

Looks really nice, thanks a lot! I see that you also addressed the issue with the unit tests raised in #192 . To be honest, I don't know why exactly the results deviate from the original values there. I suspect that I changed something a while ago (maybe a bugfix) and since the mlxtend.plotting package wasn't part of the unit tests on CI, it probably wasn't updated in the testing scripts. Everything looks correct to me know, and if there's no other concern from your side, I'd be happy to adjust the documentation and merge this (and re-enable mlxtend.plotting in the Travis CI tests so that sth like that doesn't happen again in future :)).

[soumyadsanyal]

@rasbt sounds good to me! :)

[rasbt]

Sorry, but on a second thought, I think we can simplify this greatly. E.g., there's a check_scoring function in scikit-learn that we could use to get the "right" scorer for a given metric/estimator automatically.

Also, maybe it would be good to rename clf to estimator. Then we could add the following docstring for scoring:

"""
...
    scoring : str, callable, or None (default=None)
        Scoring-function based on scikit-learn:
        - None: Uses the estimator's default scoring function 
        - String (see http://scikit-learn.org/stable/modules/
                  model_evaluation.html#common-cases-predefined-values)
        - 'misclassification error': 1-accuracy
        - A scorer, callable object / function 
        with signature `scorer(estimator, X, y)`.
...
"""

In brief, the check_scoring could be used to create the scorer like so:

    from sklearn.metrics.scorer import check_scoring
    scorer = check_scoring(estimator, scoring=scoring)

Here's a more detailed example demonstrating how this works:

from sklearn.metrics.scorer import check_scoring
from sklearn import datasets
from sklearn.model_selection import train_test_split

iris = datasets.load_iris()
X = iris.data
y = iris.target
X_train, X_test, y_train, y_test = (train_test_split(X, y,
                                    train_size=0.6, random_state=2))

clf = LogisticRegression(random_state=1)
y_pred = clf.fit(X_train, y_train).predict(X_test)

scorer1 = check_scoring(clf, scoring=None)
score1 = scorer(clf, X_test, y_test)

scorer2 = check_scoring(clf, scoring='accuracy')
score2 = scorer(clf, X_test, y_test)

assert score1 == score2

print('Accuracy', score1)

Accuracy 0.966666666667

And for the misclassification error:

if 'misclassification error':
    scorer3 = check_scoring(clf, scoring='accuracy')
    score3 = 1 - scorer(clf, X_test, y_test)
    print('misclassification error', score3)

Sorry about the "change of mind," but I think using check_scoring would make things more robust and "cleaner". What do you think?

[soumyadsanyal]

@rasbt I definitely agree that would be better, I hadn't noticed the check_scoring method; great catch! I'd be happy to implement this in a couple of days if there's no rush to merge sooner than that.

[rasbt]

Thanks, and there's definitely no rush :)

[coveralls]

Coverage remained the same at 89.211% when pulling c92f5e4dba82cf7347ec807133ceeda55dd6ef9c on soumyadsanyal:soumyadsanyal/learning_curves_predict_proba into c6738f05db153eba25336bf9c2a1506ec4ab7508 on rasbt:master.

[soumyadsanyal]

... sorry about the delay on this, was swamped with a few things. Will wrap this up shortly!

[rasbt]

Thanks for the update, and again, no worries regarding the delay. Really appreciate the PR!

[soumyadsanyal]

TODO: maybe guards / try-catch in complement_scorer for scorer methods evaluating within [0,1].

But intend to add tests.

[coveralls]

Coverage increased (+0.02%) to 91.304% when pulling 60420dd4e3664e6900469c19386e7049bdde379c on soumyadsanyal:soumyadsanyal/learning_curves_predict_proba into 975c59805aedbb63bcc95f0f8d5a3cd7d0e630a5 on rasbt:master.

[rasbt]

Thanks for the update!

maybe guards / try-catch in complement_scorer for scorer methods evaluating within [0,1]

I think that would be a good idea. Another thing, and it was a bad choice in hindsight, the default is "misclassification error" ... I'd say that's probably the most common use case for learning curves in literature, however, there would be a problem now if the estimator is a regressor. Maybe, we should change the default to None, which then uses the estimators default, and if the estimator does not have a default, it would raise a warning so that a user has to specify the metric. What do you think?

[soumyadsanyal]

👍 Yep, that sounds good :). I'll update very shortly.

[coveralls]

Coverage increased (+0.02%) to 91.304% when pulling c5737cbf7212fda9138e14c6cf5386f12815b110 on soumyadsanyal:soumyadsanyal/learning_curves_predict_proba into 827ba35bba2e5eb63e8d002a3b46346734d60066 on rasbt:master.

[rasbt]

Thanks for the PR, I see that it is passing now :). Could you maybe include a regressor in the unit tests, just to make sure that it works as intended?

PS: I am happy to take care of updating the documentation (the API doc, the Jupyter Notebook, and the Changelog) if you like (it would just require to check the "Allow edits from maintainers." option in the lower right corner)