-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
TST: Fail on warning #22699
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
TST: Fail on warning #22699
Changes from 39 commits
9480916
bd5a419
36279c9
ad69f50
43d7a78
4919b0a
2804192
7ad249a
953fde1
d2dd0af
b813e41
3baba66
3067ed1
614f514
37a3d39
80ce447
61beba7
cf1ff63
a9a672d
da4961a
bed003b
8bd8711
ba46eef
3b0b9b0
e85d8d7
9eeb6a6
a89721e
9813b4c
b7f0198
dfc767c
51f77b5
5a1b8ee
83cd9ae
d10a0cc
cf60217
61587ec
cfb5ae2
2591677
1d39520
5f0eefe
7f618e9
4990fc2
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 |
---|---|---|
|
@@ -632,6 +632,14 @@ Otherwise, you need to do it manually: | |
warnings.warn('Use new_func instead.', FutureWarning, stacklevel=2) | ||
new_func() | ||
|
||
You'll also need to | ||
|
||
1. write a new test that asserts a warning is issued when calling with the deprecated argument | ||
2. Update all of pandas existing tests and code to use the new argument | ||
|
||
See :ref:`contributing.warnings` for more. | ||
|
||
|
||
.. _contributing.ci: | ||
|
||
Testing With Continuous Integration | ||
|
@@ -859,6 +867,55 @@ preferred if the inputs or logic are simple, with Hypothesis tests reserved | |
for cases with complex logic or where there are too many combinations of | ||
options or subtle interactions to test (or think of!) all of them. | ||
|
||
.. _contributing.warnings: | ||
|
||
Testing Warnings | ||
~~~~~~~~~~~~~~~~ | ||
|
||
By default, one of pandas CI workers will fail if any unhandled warnings are emitted. | ||
|
||
If your change involves checking that a warning is actually emitted, use | ||
``tm.assert_produces_warning(ExpectedWarning)``. | ||
|
||
|
||
.. code-block:: python | ||
|
||
with tm.assert_prodcues_warning(FutureWarning): | ||
df.some_operation() | ||
|
||
We prefer this to the ``pytest.warns`` context manager because ours checks that the warning's | ||
stacklevel is set correctly. The stacklevel is what ensure the *user's* file name and line number | ||
is printed in the warning, rather than something internal to pandas. It represents the number of | ||
function calls from user code (e.g. ``df.some_operation()``) to the function that actually emits | ||
the warning. | ||
|
||
If you have a test that would emit a warning, but you aren't actually testing the | ||
warning itself (say because it's going to be removed in the future, or because we're | ||
matching a 3rd-party library's behavior), then use ``pytest.mark.filterwarnings`` to | ||
ignore the error. | ||
|
||
.. code-block:: python | ||
|
||
@pytest.mark.filterwarnings("ignore:msg:category") | ||
def test_thing(self): | ||
... | ||
|
||
If the test generates a warning of class ``category`` whose message starts | ||
with ``msg``, the warning will be ignored and the test will pass. | ||
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. Should we also document the
when you only want to ignore warnings in a part of the test? |
||
|
||
If you need finer-grained control, you can use Python's usual | ||
`warnings module <https://docs.python.org/3/library/warnings.html>`__ | ||
to control whether a warning is ignored / raised at different places within | ||
a single test. | ||
|
||
.. code-block:: python | ||
|
||
with warch.catch_warnings(): | ||
warnings.simplefilter("ignore", FutureWarning) | ||
# Or use warnings.filterwarnings(...) | ||
|
||
Alternatively, consider breaking up the unit test. | ||
|
||
|
||
Running the test suite | ||
---------------------- | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,7 @@ | |
import struct | ||
import inspect | ||
from collections import namedtuple | ||
import collections | ||
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. Using ABCs from 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. Assuming the subsequent changes didn't generate any failures does this point to a potential lack of compatibility test coverage? 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 don't follow 100%, but I don't think so. On 3.7, you basically couldn't import pandas when |
||
|
||
PY2 = sys.version_info[0] == 2 | ||
PY3 = sys.version_info[0] >= 3 | ||
|
@@ -135,6 +136,11 @@ def lfilter(*args, **kwargs): | |
|
||
from importlib import reload | ||
reload = reload | ||
Hashable = collections.abc.Hashable | ||
Iterable = collections.abc.Iterable | ||
Mapping = collections.abc.Mapping | ||
Sequence = collections.abc.Sequence | ||
Sized = collections.abc.Sized | ||
|
||
else: | ||
# Python 2 | ||
|
@@ -190,6 +196,12 @@ def get_range_parameters(data): | |
|
||
reload = builtins.reload | ||
|
||
Hashable = collections.Hashable | ||
Iterable = collections.Iterable | ||
Mapping = collections.Mapping | ||
Sequence = collections.Sequence | ||
Sized = collections.Sized | ||
|
||
if PY2: | ||
def iteritems(obj, **kw): | ||
return obj.iteritems(**kw) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,6 +59,7 @@ def cmp_method(self, other): | |
# numpy will show a DeprecationWarning on invalid elementwise | ||
# comparisons, this will raise in the future | ||
with warnings.catch_warnings(record=True): | ||
warnings.filterwarnings("ignore", "elementwise", FutureWarning) | ||
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. Not fully related to this change, but do we actually want to ignore the warning here? Shouldn't we rather fix it if numpy will do something differently in the future? 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. Added to #22698 |
||
with np.errstate(all='ignore'): | ||
result = op(self.values, np.asarray(other)) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ | |
|
||
from pandas._libs.lib import infer_dtype | ||
from pandas.util._decorators import cache_readonly | ||
from pandas.compat import u, range | ||
from pandas.compat import u, range, string_types | ||
from pandas.compat import set_function_name | ||
|
||
from pandas.core.dtypes.cast import astype_nansafe | ||
|
@@ -147,6 +147,11 @@ def coerce_to_array(values, dtype, mask=None, copy=False): | |
dtype = values.dtype | ||
|
||
if dtype is not None: | ||
if (isinstance(dtype, string_types) and | ||
(dtype.startswith("Int") or dtype.startswith("UInt"))): | ||
# Avoid DeprecationWarning from NumPy about np.dtype("Int64") | ||
# https://github.com/numpy/numpy/pull/7476 | ||
dtype = dtype.lower() | ||
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. @jreback could you take a close look here. I think you ran into this when writing a test. 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. Is there a test that causes this warning? (I can see this locally, but I didn't see it appearing when running tests) 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. Hmm, ignore previous comment, I thought pytest automatically shows all warnings, which is not the case. Shouldn't we edit out 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. This is a deprecation warning from NumPy right now, which is filtered interactive, but pytest removes that filter by default. Some of the 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. oh I didn't know numpy did this:
I guess so, thanks 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. Yeah. I'm waiting to hear back on numpy/numpy#7476 (comment) If |
||
if not issubclass(type(dtype), _IntegerDtype): | ||
try: | ||
dtype = _dtypes[str(np.dtype(dtype))] | ||
|
@@ -507,7 +512,8 @@ def cmp_method(self, other): | |
|
||
# numpy will show a DeprecationWarning on invalid elementwise | ||
# comparisons, this will raise in the future | ||
with warnings.catch_warnings(record=True): | ||
with warnings.catch_warnings(): | ||
warnings.filterwarnings("ignore", "elementwise", FutureWarning) | ||
with np.errstate(all='ignore'): | ||
result = op(self._data, other) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -758,7 +758,7 @@ def aggregate(self, func_or_funcs, *args, **kwargs): | |
if isinstance(func_or_funcs, compat.string_types): | ||
return getattr(self, func_or_funcs)(*args, **kwargs) | ||
|
||
if isinstance(func_or_funcs, collections.Iterable): | ||
if isinstance(func_or_funcs, compat.Iterable): | ||
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. in theory we could add a lint rule to avoid using 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 we're close enough to py3-only that we'll be OK. Writing that regex would be a bit fiddly since we'd need to enumerate all the ABCs. 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. true, certainly a follow up issue is ok |
||
# Catch instances of lists / tuples | ||
# but not the class list / tuple itself. | ||
ret = self._aggregate_multiple_funcs(func_or_funcs, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -98,6 +98,7 @@ def cmp_method(self, other): | |
# numpy will show a DeprecationWarning on invalid elementwise | ||
# comparisons, this will raise in the future | ||
with warnings.catch_warnings(record=True): | ||
warnings.filterwarnings("ignore", "elementwise", FutureWarning) | ||
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. What's the point in using 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. laziness / not wanting to introduce copy-paste errors. New code should essentially always use marks, and I can make the changes here if you want. I mostly cleaned up Panel to remove these 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. Makes sense. Maybe not a hard rule on this change but it would be helpful to use this instead in functions where there is a big diff |
||
with np.errstate(all='ignore'): | ||
result = op(self.values, np.asarray(other)) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3490,6 +3490,7 @@ def _putmask_smart(v, m, n): | |
|
||
# we ignore ComplexWarning here | ||
with warnings.catch_warnings(record=True): | ||
warnings.simplefilter("ignore", np.ComplexWarning) | ||
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 the context manager restore all of the warnings filters after? (I guess that is the point?) 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. Yeah, that's the idea. Ideally libraries shouldn't modify the global warnings registry without good cause. |
||
nn_at = nn.astype(v.dtype) | ||
|
||
# avoid invalid dtype comparisons | ||
|
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.
you may want to mention that we will actually fail if pytest.warns will fail the linter :>