Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

Warn default value of min_padding_length #2309

Merged
merged 8 commits into from Jan 13, 2019

Conversation

WrRan
Copy link
Contributor

@WrRan WrRan commented Jan 8, 2019

Fixes #2287.

f"which can cause some subtle bugs (more info see {url}). "
"Strongly recommend to set a value, usually the maximum size "
"of the convolutional layer size when using CnnEncoder.",
FutureWarning)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think FutureWarning is right here - nothing is going to change in the future, it's just a warning that the user might have buggy code. Maybe the default of UserWarning is correct here?

@@ -1,8 +1,6 @@
# pylint: disable=no-self-use,invalid-name
from collections import defaultdict

import warnings

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

Copy link
Contributor

@matt-gardner matt-gardner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks good. There is one other important thing to fix, though: this adds a whole bunch of new warnings thrown in our test suite. Those need to be fixed or squelched before this can be merged. Also, in your other PR, UserWarning and FutureWarning should be treated as an error, just like DeprecationWarning is.

@WrRan
Copy link
Contributor Author

WrRan commented Jan 9, 2019

Got it. I am gonna to fix it.

@WrRan
Copy link
Contributor Author

WrRan commented Jan 12, 2019

Please have a look. I think related warnings are clear now.

Copy link
Contributor

@matt-gardner matt-gardner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@matt-gardner matt-gardner merged commit 632b14c into allenai:master Jan 13, 2019
@WrRan WrRan deleted the warn_min_padding_length branch March 16, 2019 14:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants