Skip to content
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

Add classes_ attribute NeuralNetClassifier #546

Merged
merged 2 commits into from Nov 6, 2019

Conversation

@BenjaminBossan
Copy link
Collaborator

BenjaminBossan commented Oct 13, 2019

This is inferred from y default but can be overridden by passing
classes explicitly during initialization.

Also, add more rigorous tests to classifiers and regressor.
That is to catch errors with specific attributes being set on those
that are not set on NeuralNet itself. This can potentially mess with
cloning, which is why it should be tested explicitly.

Addresses #465, #486

BenjaminBossan added 2 commits Oct 13, 2019
This is inferred from y default but can be overridden by passing
classes explicitly during initialization.
That is to catch errors with specific attributes being set on those
that are not set on NeuralNet itself. This can potentially mess with
cloning, which is why it should be tested explicitly.
@BenjaminBossan BenjaminBossan requested review from thomasjpfan and ottonemo Oct 13, 2019
@BenjaminBossan BenjaminBossan self-assigned this Oct 13, 2019
@BenjaminBossan BenjaminBossan changed the title Add `classes_` attribute `NeuralNetClassifier` Add classes_ attribute NeuralNetClassifier Oct 13, 2019
Copy link
Member

thomasjpfan left a comment

We need to make sure that predict will return the classes used in fit.

@@ -99,6 +123,8 @@ def check_data(self, X, y):
"``iterator_train`` and ``iterator_valid`` parameters "
"respectively.")
raise ValueError(msg)
if y is not None:
self.classes_inferred_ = np.unique(y)

This comment has been minimized.

Copy link
@thomasjpfan

thomasjpfan Oct 15, 2019

Member

classes_inferred_ can be private.

This comment has been minimized.

Copy link
@BenjaminBossan

BenjaminBossan Oct 16, 2019

Author Collaborator

That's the first thing I did, but this has unintended side effects on clone, resulting in it failing the consistency check. This might be something we should investigate further but I thought it won't hurt to have this attribute there. I wouldn't document it in case we do want to remove it later.

@githubnemo

This comment has been minimized.

Copy link

githubnemo commented Nov 2, 2019

We need to make sure that predict will return the classes used in fit.

This doesn't feel strictly necessary to me. Can you elaborate?

@githubnemo

This comment has been minimized.

Copy link

githubnemo commented Nov 4, 2019

I would recommend merging this and, if it turns out to be an common issue, add checks in the inference step for class consistency.

@thomasjpfan

This comment has been minimized.

Copy link
Member

thomasjpfan commented Nov 6, 2019

We need to make sure that predict will return the classes used in fit.

That's if we really want to comply with the API. For example:

from sklearn.datasets import load_iris
from sklearn.linear_model import LogisticRegression
X, y = [[0], [1]], ['dog', 'cat']
clf = LogisticRegression(random_state=0, solver='lbfgs').fit(X, y)

clf.classes_
clf.classes_
# array(['cat', 'dog'], dtype='<U3')

clf.predict(X)
# array(['dog', 'cat'], dtype='<U3')
@BenjaminBossan

This comment has been minimized.

Copy link
Collaborator Author

BenjaminBossan commented Nov 6, 2019

I forgot this was possible, it fits a LabelEncoder internally, right? Is this supported by all classifiers?

Anyway, I'm not 100% convinced we need that, and would thus propose to postpone the decision.

@thomasjpfan

This comment has been minimized.

Copy link
Member

thomasjpfan commented Nov 6, 2019

Yea all classifiers do something like this.

Personally, I would rather not support strings here, because it makes things fairly complicated. Restricting classes_ to integers should be good enough for most users.

@thomasjpfan

This comment has been minimized.

Copy link
Member

thomasjpfan commented Nov 6, 2019

I am suggesting we check that classes_ are ints if it was passed in __init__ during fit?

@ottonemo ottonemo merged commit 8dd58ef into master Nov 6, 2019
4 checks passed
4 checks passed
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@BenjaminBossan

This comment has been minimized.

Copy link
Collaborator Author

BenjaminBossan commented Nov 6, 2019

I am suggesting we check that classes_ are ints if it was passed in init during fit?

Too late :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.