-
Notifications
You must be signed in to change notification settings - Fork 50
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
ENH: Add default losses to KerasClassifier and KerasRegressor #208
base: master
Are you sure you want to change the base?
Changes from 4 commits
4be5d0c
dccd92b
1d80b57
1f45285
9358d6e
6cc112e
80618bf
60b2404
dcb0823
0faadd9
0449481
ed4c1f5
c58ec74
e73710d
4e7e09f
2e830ff
e1ea339
36e6499
8310834
6ee8b50
b88b74e
3a3a536
9808cf2
d0147ac
7243995
9735974
8cc0474
b0229c5
dccfc5e
d2e23cb
9c3af6b
5121131
0dfa526
7b379d7
e4338fc
ca69f2e
2ac57e0
0de8abe
0cf7610
d4c3eea
7fab517
59e7012
0386e4e
3a46538
8f2b00b
e80338b
7e23480
94df48a
35e1a6c
f092b7a
5af8b4c
7b38bc8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
import numpy as np | ||
import pytest | ||
import tensorflow as tf | ||
|
||
from sklearn.datasets import make_classification | ||
|
||
from scikeras.wrappers import KerasClassifier | ||
|
||
|
||
N_CLASSES = 4 | ||
FEATURES = 8 | ||
n_eg = 100 | ||
X = np.random.uniform(size=(n_eg, FEATURES)).astype("float32") | ||
y = np.random.choice(N_CLASSES, size=n_eg).astype(int) | ||
|
||
|
||
def clf(single_output=False): | ||
model = tf.keras.Sequential() | ||
model.add(tf.keras.layers.Input(shape=(FEATURES,))) | ||
model.add(tf.keras.layers.Dense(FEATURES)) | ||
|
||
if single_output: | ||
model.add(tf.keras.layers.Dense(1)) | ||
else: | ||
model.add(tf.keras.layers.Dense(N_CLASSES)) | ||
|
||
return model | ||
|
||
|
||
def test_classifier_only_model_specified(): | ||
""" | ||
This tests uses cases where KerasClassifier works with the default loss. | ||
It works for the following cases: | ||
|
||
* binary classification | ||
* one hot classification | ||
* single class classification | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean we do not support 1 output with multiple classes? Am I getting confused by the usage of outputs vs. output units. The most common way to set up single target multi-class problems in Keras is with def clf():
model = tf.keras.Sequential()
model.add(tf.keras.layers.Input(shape=(FEATURES,)))
model.add(tf.keras.layers.Dense(N_CLASSES, activation="softmax"))
model.compile(loss="cce") # or "scce"
return model Would this use no longer be supported? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I see now. This should work! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've made the uses cases where this works more clear in 9358d6e. It works for all major use cases. This PR simply changes the default loss; it doesn't change compatibility in any way. |
||
|
||
""" | ||
est = KerasClassifier(model=clf) | ||
est.partial_fit(X, y=y) | ||
stsievert marked this conversation as resolved.
Show resolved
Hide resolved
|
||
assert est.current_epoch == 1 | ||
|
||
for y2 in [ | ||
np.random.choice(2, size=len(X)).astype(int), | ||
(np.random.choice(2, size=len(X)).astype(int) * 2 - 1), | ||
np.ones(len(X)).astype(int), | ||
np.zeros(len(X)).astype(int), | ||
]: | ||
est = KerasClassifier(model=clf, model__single_output=True) | ||
est.partial_fit(X, y=y2) | ||
assert est.current_epoch == 1 | ||
|
||
|
||
def test_classifier_raises_for_single_output_with_multiple_classes(): | ||
""" | ||
KerasClassifier does not work with one output and multiple classes | ||
in the target (duh). | ||
""" | ||
est = KerasClassifier(model=clf, model__single_output=True) | ||
msg = ( | ||
"The model is configured to have one output, but the " | ||
"loss='categorical_crossentropy' is expecting multiple outputs " | ||
stsievert marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
with pytest.raises(ValueError, match=msg): | ||
est.partial_fit(X, y) | ||
assert est.current_epoch == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it should live in
_check_model_compatibility
, or they should be merged in some way.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error message only provides marginal utility: it protects against cases when the model has one output but there are multiple classes.
It can not go in
_check_model_compatibility
; I wait for an error to be raised before issuing this warning (otherwise a model with a single output raises an error).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Is there a specific error message we can check for, like
if "some Keras error" in str(e)
?Is this necessary?
model_
should always have anoutputs
attribute, except in the case described in #207, but that should be a separate check/error.Can you clarify what you mean by a loss expecting a number of outputs? My understanding is that Keras "broadcasts" losses to outputs, so if you give it a scalar loss (ie..
loss="bce"
) with 2 outputs (i.e.len(model_.outputs) == 2
), it will implicitly compile the model withloss=[original_loss] * len(outputs)
. But you can actually map losses to outputs manually, by passingloss=["bce", "mse"]
orloss={"out1": "bce", "out2": "mse"}
. From the tests, it seems like by "loss is expecting multiple outputs" you mean that there is a single output unit but multiple classes, which I feel like could be confused with the above concept of configuring a loss for multiple outputs.I'm also curious about the iteration through outputs (
1 in {o.shape[1] for o in self.model_.outputs}
). SciKeras does not support >1 output out of the box (users need to overridetarget_encoder
) so it seems a bit strange to try to account for that when using the default loss. I feel that using the default loss should only be supported for the simple single-output cases thattarget_encoder
supports.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a side note: I think giving users better errors and validating their inputs like you are doing here can be a very valuable part of SciKeras, but currently it is done in an ad-hoc manner via
_check_model_compatibility
, etc. I think if we add more of these types of things, it would be nice to have an organized interface for it. I opened #209 to try to brainstorm ideas for this.