-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Tests for Deprecate SparseArray for python 3.6 and 3.7 and fixes to other deprecation tests #30799
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
Changes from 7 commits
4ace5eb
76dbae4
2bbc831
def8d28
a32d4dc
ea7b959
385f6d3
f5d0357
2e0bab0
52a7d03
5c59f84
461b0fc
f1b89f4
f08ffc9
aa73611
4b7e4bf
c31afa1
26dc90a
7861c98
7947a81
d78ecea
7cce2c7
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 |
---|---|---|
|
@@ -43,7 +43,7 @@ class TestPDApi(Base): | |
] | ||
|
||
# these are already deprecated; awaiting removal | ||
deprecated_modules: List[str] = [] | ||
deprecated_modules: List[str] = ["np", "datetime"] | ||
|
||
# misc | ||
misc = ["IndexSlice", "NaT", "NA"] | ||
|
@@ -90,15 +90,17 @@ class TestPDApi(Base): | |
"UInt64Dtype", | ||
"NamedAgg", | ||
] | ||
if not compat.PY37: | ||
classes.extend(["Panel", "SparseSeries", "SparseDataFrame", "SparseArray"]) | ||
deprecated_modules.extend(["np", "datetime"]) | ||
|
||
# these are already deprecated; awaiting removal | ||
deprecated_classes: List[str] = [] | ||
|
||
# these should be deprecated in the future | ||
deprecated_classes_in_future: List[str] = [] | ||
deprecated_classes_in_future: List[str] = ["SparseArray"] | ||
|
||
if not compat.PY37: | ||
classes.extend(["Panel", "SparseSeries", "SparseDataFrame"]) | ||
# deprecated_modules.extend(["np", "datetime"]) | ||
# deprecated_classes_in_future.extend(["SparseArray"]) | ||
|
||
# external modules exposed in pandas namespace | ||
modules: List[str] = [] | ||
|
@@ -201,41 +203,46 @@ class TestPDApi(Base): | |
|
||
def test_api(self): | ||
|
||
self.check( | ||
pd, | ||
checkthese = ( | ||
self.lib | ||
+ self.misc | ||
+ self.modules | ||
+ self.deprecated_modules | ||
+ self.classes | ||
+ self.deprecated_classes | ||
+ self.deprecated_classes_in_future | ||
+ self.funcs | ||
+ self.funcs_option | ||
+ self.funcs_read | ||
+ self.funcs_json | ||
+ self.funcs_to | ||
+ self.deprecated_funcs_in_future | ||
+ self.deprecated_funcs | ||
+ self.private_modules, | ||
self.ignored, | ||
+ self.private_modules | ||
) | ||
if not compat.PY37: | ||
checkthese.extend( | ||
self.deprecated_modules | ||
+ self.deprecated_classes | ||
+ self.deprecated_classes_in_future | ||
+ self.deprecated_funcs_in_future | ||
+ self.deprecated_funcs | ||
) | ||
self.check(pd, checkthese, self.ignored) | ||
|
||
def test_depr(self): | ||
deprecated = ( | ||
deprecated_list = ( | ||
self.deprecated_modules | ||
+ self.deprecated_classes | ||
+ self.deprecated_classes_in_future | ||
+ self.deprecated_funcs | ||
+ self.deprecated_funcs_in_future | ||
) | ||
for depr in deprecated: | ||
for depr in deprecated_list: | ||
with tm.assert_produces_warning(FutureWarning): | ||
if compat.PY37: | ||
getattr(pd, depr) | ||
elif depr == "datetime": | ||
deprecated = getattr(pd, "__Datetime") | ||
deprecated().__getattr__(dir(pd.datetime)[-1]) | ||
deprecated().__getattr__(dir(pd.datetime.datetime)[-1]) | ||
elif depr == "SparseArray": | ||
deprecated = getattr(pd, depr) | ||
deprecated([]) | ||
else: | ||
deprecated = getattr(pd, depr) | ||
deprecated.__getattr__(dir(deprecated)[-1]) | ||
|
@@ -245,9 +252,10 @@ def test_datetime(): | |
from datetime import datetime | ||
import warnings | ||
|
||
with warnings.catch_warnings(): | ||
warnings.simplefilter("ignore", FutureWarning) | ||
assert datetime(2015, 1, 2, 0, 0) == pd.datetime(2015, 1, 2, 0, 0) | ||
if compat.PY37: | ||
with warnings.catch_warnings(): | ||
warnings.simplefilter("ignore", FutureWarning) | ||
assert datetime(2015, 1, 2, 0, 0) == pd.datetime(2015, 1, 2, 0, 0) | ||
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. Why only test this for python 3.7? The equality should also hold for python 3.6 ? 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. See my analysis above. I couldn't figure out how to make 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. But this test should also pass when there is no warning? (the test doesn't assert there is a warning, only that the functionality is still working) 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. The issue with python 3.6 and 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 a correct result is more important right now than having the deprecation warning in all cases. |
||
|
||
|
||
def test_np(): | ||
|
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.
I am not sure we can do it this way for SparseArray, as that breaks isinstance checks
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.
To confirm, you mean something like
will be false? Since it's an instance of the parent
pd.arrays.SparseArray
, not the childpd.SparseArray
?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.
Yes.
(in general, deprecating moving classes is annoying)
For
datetime
andnp
the same is true of course, but I think it is much less likely that someone will do that.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 is again another issue with python 3.6 handling the deprecation of a module attribute. That's why they implemented PEP 562 (https://www.python.org/dev/peps/pep-0562/) for python 3.7. Making a reference using python 3.6 to
pd.SparseArray
(without a constructor) produce a warning would be quite involved (see the Rationale in the PEP discussion), and I don't think it is worth introducing that level of complexity to pandas to handle deprecation messages of a class for users of an older version of python.It's why I suggested above to merge in this pull request, and we can leave this as an open issue.
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.
I know it's an issue with python 3.6, but that we still support 3.6 now is a reality we have to live with.
So my suggestion would be to simply don't try to raise a warning in python 3.6, and just import the existing class in that case (by the time we are going to enforce this deprecation, we maybe won't support py3.6 anymore, so then that is less of an issue). Or simply don't deprecate pd.SparseArray at all.
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.
That's probably my preference until we support just 3.7+.
Though @Dr-Irv this PR is also fixing other issues with the other deprecations on master? Taking a closer look now.
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.
@TomAugspurger Yes, there were other issues with deprecations on master that I addressed, especially in the testing code.