Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
ENH: Add default losses to KerasClassifier and KerasRegressor #208
Changes from 13 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
There are no files selected for viewing
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.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.
Is the assertion that the "long" name for the loss was used in the model necessary here? I don't see the same assertion for classifiers.
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 assert statement is present to make sure the
BaseWrapper.loss
is mirror inBaseWrapper.model_.loss
. I'll add a test forKerasClassifier
too.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.
Maybe this is good use case for
scikeras.utils.loss_name
?