Skip to content

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.

Sign up for GitHub

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

Merged
merged 22 commits into from
Jan 9, 2020
Merged

Tests for Deprecate SparseArray for python 3.6 and 3.7 and fixes to other deprecation tests #30799

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
4ace5eb
fix deprecation test for python 3.7
Dr-Irv Jan 7, 2020
76dbae4
sparsearray OK for 3.6
Dr-Irv Jan 7, 2020
2bbc831
handle 3.6/3.7 diffs
Dr-Irv Jan 7, 2020
def8d28
formatted for black
Dr-Irv Jan 7, 2020
a32d4dc
Merge remote-tracking branch 'upstream/master' into sparsedepr
Dr-Irv Jan 7, 2020
ea7b959
split test_api for 3.6 vs 3.7
Dr-Irv Jan 7, 2020
385f6d3
Merge branch 'sparsedepr' of https://github.com/Dr-Irv/pandas into sp…
Dr-Irv Jan 7, 2020
f5d0357
Merge branch 'sparsedepr' of https://github.com/Dr-Irv/pandas into sp…
Dr-Irv Jan 7, 2020
2e0bab0
datetime fixes
Dr-Irv Jan 7, 2020
52a7d03
Merge branch 'sparsedepr' of https://github.com/Dr-Irv/pandas into sp…
Dr-Irv Jan 7, 2020
5c59f84
make datetime work with 3.6
Dr-Irv Jan 8, 2020
461b0fc
Merge remote-tracking branch 'upstream/master' into sparsedepr
Dr-Irv Jan 8, 2020
f1b89f4
use instancecheck. Modify SparseArray for 3.6 to use metaclass pattern
Dr-Irv Jan 8, 2020
f08ffc9
Merge branch 'sparsedepr' of https://github.com/Dr-Irv/pandas into sp…
Dr-Irv Jan 8, 2020
aa73611
Merge remote-tracking branch 'upstream/master' into sparsedepr
Dr-Irv Jan 8, 2020
4b7e4bf
Merge branch 'sparsedepr' of https://github.com/Dr-Irv/pandas into sp…
Dr-Irv Jan 8, 2020
c31afa1
fix mypy issue for methods with no arguments
Dr-Irv Jan 8, 2020
26dc90a
Merge remote-tracking branch 'upstream/master' into sparsedepr
Dr-Irv Jan 9, 2020
7861c98
snake_case and better returns on isinstance
Dr-Irv Jan 9, 2020
7947a81
shorten up imports for datetime and SparseArray
Dr-Irv Jan 9, 2020
d78ecea
revert import simplification to make mypy happy
Dr-Irv Jan 9, 2020
7cce2c7
Merge remote-tracking branch 'upstream/master' into sparsedepr
Dr-Irv Jan 9, 2020
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 29 additions & 3 deletions pandas/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,11 +319,37 @@ def __getattr__(self, item):
except AttributeError:
raise AttributeError(f"module datetime has no attribute {item}")

datetime = __Datetime().datetime
datetime = __Datetime()

class SparseArray:
pass
class __SparseArray(pandas.core.arrays.sparse.SparseArray):
Copy link
Member

@jorisvandenbossche jorisvandenbossche Jan 8, 2020

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
Copy link
Contributor

@TomAugspurger TomAugspurger Jan 8, 2020

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

isinstance(pd.array([1, 2, 3], dtype="Sparse"), pd.SparseArray)

will be false? Since it's an instance of the parent pd.arrays.SparseArray, not the child pd.SparseArray?
Copy link
Member

@jorisvandenbossche jorisvandenbossche Jan 8, 2020

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 and np the same is true of course, but I think it is much less likely that someone will do that.
Copy link
Contributor Author

@Dr-Irv Dr-Irv Jan 8, 2020

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.
Copy link
Member

@jorisvandenbossche jorisvandenbossche Jan 8, 2020

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.
Copy link
Contributor

@TomAugspurger TomAugspurger Jan 8, 2020

Choose a reason for hiding this comment

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

Or simply don't deprecate pd.SparseArray at all.

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.
Copy link
Contributor Author

@Dr-Irv Dr-Irv Jan 8, 2020

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.
def __warnSparseArray(self):
import warnings

warnings.warn(
"The pandas.SparseArray class is deprecated "
"and will be removed from pandas in a future version. "
"Use pandas.arrays.SparseArray instead.",
FutureWarning,
stacklevel=3,
)

def __init__(
self,
data,
sparse_index=None,
index=None,
fill_value=None,
kind="integer",
dtype=None,
copy=False,
):
self.__warnSparseArray()
super().__init__(data, sparse_index, index, fill_value, kind, dtype, copy)

def __getattr__(self, name):
return super().__getattribute__(name)

SparseArray = __SparseArray

# module level doc-string
__doc__ = """
Expand Down
48 changes: 28 additions & 20 deletions pandas/tests/api/test_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
Expand Down Expand Up @@ -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] = []
Expand Down Expand Up @@ -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])
Expand All @@ -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)
Copy link
Member

@jorisvandenbossche jorisvandenbossche Jan 8, 2020

Choose a reason for hiding this comment

The 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 ?
Copy link
Contributor Author

@Dr-Irv Dr-Irv Jan 8, 2020

Choose a reason for hiding this comment

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

See my analysis above. I couldn't figure out how to make pd.datetime have a deprecation message when doing pd.datetime(2015,1,2,0,0) and also support pd.datetime.now() with a deprecation message when using python 3.6.
Copy link
Member

@jorisvandenbossche jorisvandenbossche Jan 8, 2020

Choose a reason for hiding this comment

The 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)
Copy link
Contributor Author

@Dr-Irv Dr-Irv Jan 8, 2020

Choose a reason for hiding this comment

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

The issue with python 3.6 and using pd.datetime(2015,1,2,0,0) is that the implementation in this PR throws an exception. Current master just succeeds without any warning at all.
Copy link
Member

@jorisvandenbossche jorisvandenbossche Jan 8, 2020

Choose a reason for hiding this comment

The 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():
Expand Down