-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 drop_last option to method fit of PyTorchClassifier #1883
Conversation
Signed-off-by: Beat Buesser <beat.buesser@ie.ibm.com>
Signed-off-by: Beat Buesser <beat.buesser@ie.ibm.com>
Codecov Report
@@ Coverage Diff @@
## dev_1.12.2 #1883 +/- ##
==============================================
- Coverage 85.90% 85.73% -0.17%
==============================================
Files 248 248
Lines 23312 23491 +179
Branches 4212 4280 +68
==============================================
+ Hits 20027 20141 +114
- Misses 2224 2273 +49
- Partials 1061 1077 +16
|
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.
Hi Beat,
This looks good - I've tested locally and the code makes sense.
Just need to fix the style-checks, line 377 is over 120 characters.
Would it be worth wrapping the model prediction in a try-except block and warning the user that a drop_last may be required if this error is caught (as the default is False)?
Something like:
# Perform prediction
try:
model_outputs = self._model(i_batch)
except ValueError as e:
if 'Expected more than 1 value per channel when training' in str(e):
logger.exception("Try dropping the last incomplete batch by setting drop_last=True.")
raise e
Signed-off-by: Beat Buesser <beat.buesser@ie.ibm.com>
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.
mypy
and pylint
failing in CI / Style checks. The drop_last option should be included in PyTorchDeRandomizedSmoothing
and PyTorchRandomizedSmoothing
fit method too?
I think the try-except block is an interesting idea. I'm running a few tests if it would slow down the model training. |
Signed-off-by: Beat Buesser <beat.buesser@ie.ibm.com>
Signed-off-by: Beat Buesser <beat.buesser@ie.ibm.com>
This pull request introduces 4 alerts and fixes 1 when merging 6e05685 into 3e3a438 - view on LGTM.com new alerts:
fixed alerts:
|
Signed-off-by: Beat Buesser <beat.buesser@ie.ibm.com>
Signed-off-by: Beat Buesser <beat.buesser@ie.ibm.com>
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.
Looks good to me!
Description
This pull request adds drop_last option to method
fit
ofPyTorchClassifier
.Fixes #1723
Type of change
Please check all relevant options.