DEPR: deprecate .asobject property #18572

topper-123 · 2017-11-30T01:09:08Z

This PR supersedes #18477.
closes #18237

[ x] xref DEPR: let's deprecate #18262
[x ] tests added / passed
[ x] passes git diff upstream/master -u -- "*.py" | flake8 --diff
[ x] whatsnew entry

Deprecate Series.asobject and DatetimeIndexOpsMixin.asobject as per discussion in #18262. DatetimeIndexOpsMixin is a mixin for DatetimeIndex, PeriodIndex and TimeDeltaIndex, so all the .asobject property will be deprecated all those classes.

Internal references to asobject have been cleaned up, so a eventual removal will be easy when that time comes.

codecov · 2017-11-30T01:47:02Z

Codecov Report

Merging #18572 into master will decrease coverage by 0.01%.
The diff coverage is 93.54%.

@@            Coverage Diff             @@
##           master   #18572      +/-   ##
==========================================
- Coverage   91.35%   91.33%   -0.02%     
==========================================
  Files         164      164              
  Lines       49802    49806       +4     
==========================================
- Hits        45496    45490       -6     
- Misses       4306     4316      +10

Flag	Coverage Δ
#multiple	`89.13% <93.54%> (-0.01%)`	⬇️
#single	`40.81% <32.25%> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/ops.py	`91.77% <ø> (ø)`	⬆️
pandas/plotting/_converter.py	`65.25% <0%> (ø)`	⬆️
pandas/core/indexes/period.py	`92.94% <100%> (ø)`	⬆️
pandas/io/formats/format.py	`96.01% <100%> (ø)`	⬆️
pandas/core/internals.py	`94.44% <100%> (-0.04%)`	⬇️
pandas/core/indexing.py	`92.8% <100%> (ø)`	⬆️
pandas/core/indexes/datetimelike.py	`96.93% <100%> (+0.02%)`	⬆️
pandas/core/indexes/datetimes.py	`95.71% <100%> (ø)`	⬆️
pandas/core/algorithms.py	`94.14% <100%> (ø)`	⬆️
pandas/core/dtypes/concat.py	`99.13% <100%> (ø)`	⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7627cca...4cb8776. Read the comment docs.

codecov · 2017-11-30T01:47:19Z

Codecov Report

Merging #18572 into master will decrease coverage by 0.04%.
The diff coverage is 94.28%.

@@            Coverage Diff             @@
##           master   #18572      +/-   ##
==========================================
- Coverage   91.46%   91.42%   -0.05%     
==========================================
  Files         157      157              
  Lines       51449    51454       +5     
==========================================
- Hits        47060    47043      -17     
- Misses       4389     4411      +22

Flag	Coverage Δ
#multiple	`89.29% <94.28%> (-0.03%)`	⬇️
#single	`40.6% <37.14%> (-0.11%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/ops.py	`91.77% <ø> (ø)`	⬆️
pandas/plotting/_converter.py	`63.44% <0%> (-1.82%)`	⬇️
pandas/io/formats/format.py	`96.01% <100%> (ø)`	⬆️
pandas/core/indexing.py	`92.81% <100%> (ø)`	⬆️
pandas/core/indexes/period.py	`92.94% <100%> (ø)`	⬆️
pandas/core/internals.py	`94.44% <100%> (-0.04%)`	⬇️
pandas/core/indexes/timedeltas.py	`91.21% <100%> (ø)`	⬆️
pandas/core/frame.py	`97.81% <100%> (-0.1%)`	⬇️
pandas/core/accessor.py	`93.75% <100%> (ø)`	⬆️
pandas/core/indexes/datetimes.py	`95.69% <100%> (ø)`	⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e56195...d61f411. Read the comment docs.

topper-123 · 2017-11-30T10:25:31Z

all is green, I think the failure on appveyoyor is unrelated to the PR.

Some points:

We discussed that asobject should just return self.astype(object). It turns out that the relevant versions of index .astype call .asobject themselves, so to avoid circular calls, I've kept a new internal ._asobject. This also avoids code duplication in the various astype methods. All internal calls use .astype(object) and not .asobject. So ._asobject is only used by astype.
The return values of the various Index.asobject is astype(object), but for Series.asobject the return value is astype(object).values. so there's a need to differentiate between the two.

jorisvandenbossche · 2017-11-30T11:03:58Z

In general: please don't open new PRs, but just update the original one.

jorisvandenbossche

Looks good to me!

One comment on the deprecation message

jorisvandenbossche · 2017-11-30T12:49:29Z

pandas/core/series.py

@@ -420,12 +420,15 @@ def get_values(self):

    @property
    def asobject(self):
-        """
+        """DEPRECATED: Use ``astype(object).values`` instead.


I know "astype(object).values" is the exact replacement, but I would maybe still recommend just "astype(object)". In many cases a Series will just be fine as well, and if the user really wants the array they can do an additional .values.

I just want to prevent people using .values when it is not needed. Maybe we can mention both as well, although that makes the warning a bit long.

don't add .values, that is just adding even more confusion.

Ok, recommend to use astype(object), but still return .astype(object).values, so the API doesn't break.

jorisvandenbossche · 2017-11-30T12:49:34Z

pandas/tests/indexes/datetimes/test_ops.py

@@ -51,15 +51,15 @@ def test_ops_properties_basic(self):
        assert s.day == 10
        pytest.raises(AttributeError, lambda: s.weekday)

-    def test_asobject_tolist(self):
+    def test_astype(self):


maybe test_astype_object ? (if it is only testing that)

jreback

need to completely remove _asobject and replace with .astype(object), this may require some small changes in def astype(...) on the Index subclasses.

w/o changing this we are just adding even more technical debt.

jreback · 2017-11-30T12:54:17Z

doc/source/whatsnew/v0.22.0.txt

@@ -89,7 +89,8 @@ Deprecations

 - ``Series.from_array`` and ``SparseSeries.from_array`` are deprecated. Use the normal constructor ``Series(..)`` and ``SparseSeries(..)`` instead (:issue:`18213`).
 - ``DataFrame.as_matrix`` is deprecated. Use ``DataFrame.values`` instead (:issue:`18458`).
-
+- ``DatetimeIndex.asobject``, ``PeriodIndex.asobject`` and ``TimeDeltaIndex.asobject`` have been deprecated. Use '.astype(object)' instead (:issue:`18572`)
+- ``Series.asobject`` has been deprecated. Use '.astype(object).values' instead (:issue:`18572`)


no need to suggest .values

jreback · 2017-11-30T12:56:05Z

pandas/core/indexes/datetimelike.py

@@ -421,15 +421,25 @@ def _isnan(self):
        return (self.asi8 == iNaT)

    @property
-    def asobject(self):


I actually want to completely remove _asobject, this just adds even more technical debt.

Jeff, is it used in astype for both DatetimeIndex, PeriodIndex and TimedeltaIndex, so it is fine to have it like this, otherwise this line would be duplicated in all three of them.

so it is fine to have it like this,

is it used in astype for both DatetimeIndex, PeriodIndex and TimedeltaIndex,

and that should be addressed as well.

no its not. much better to fix this. otherwise this is just kicking the can.

If this method is only used there, what is the problem? Then it is a perfect example of a common private helper method that our classes are full of.

much better to fix this.

Then please indicate what you mean with "fix"

topper-123 · 2017-11-30T23:23:24Z

PR updated and all is green now.

Wrt. ._asobject I felt that having a common internal method was cleaner, but its not something I feel strongly about. But I've kept it as is.

Changing it later would be trivial, as its only called from three places. so IMO its not a very large technical debt, if it even is one.

jreback · 2017-12-01T00:50:55Z

@topper-123

ok, do this

move what you are calling _asobject() and call it _box_values_as_index() and put it underneath _box_values(). eliminate all calls to it as possible, ideally you would only have the 1 remaining one in the def astype() in each of the datetimelike subclasses.

jreback · 2017-12-01T00:51:19Z

pandas/_libs/algos_common_helper.pxi.in

@@ -552,8 +552,8 @@ cpdef ensure_object(object arr):
            return arr
        else:
            return arr.astype(np.object_)
-    elif hasattr(arr, 'asobject'):
-        return arr.asobject


see what effect of removing this does

Two tests fail, both at pandas\tests\test_categorical.py:811 (parameterized tests, of categorical of dtype='timedelta64[h]' and is_ordered=True).

topper-123 · 2017-12-01T15:27:56Z

Updated.

asobject is a property. I therefore have updated _deprecations to include it. Otherwise the warning get printed in ipython everytime we tab complete.

jreback

small change, otherwise lgtm.

jreback · 2017-12-01T15:35:50Z

pandas/tests/indexes/datetimelike.py

@@ -75,3 +75,17 @@ def test_map_dictlike(self, mapper):
        expected = pd.Index([pd.NaT] * len(self.index))
        result = self.index.map(mapper([], []))
        tm.assert_index_equal(result, expected)
+
+    def test_asobject_deprecated(self):
+        d = pd.date_range('2010-01-1', periods=3)


add the issue number as a comment

use d = self.create_index()

Ok, uploaded. self.create_index() takes care of calling the correct index type?

topper-123 · 2017-12-01T21:39:53Z

All green. Please verify if last change is ok.

jreback

needs a rebase and a change. otherwise lgtm.

jreback · 2017-12-02T15:46:05Z

pandas/core/indexes/datetimelike.py

@@ -242,6 +242,14 @@ def _box_values(self, values):
        """
        return lib.map_infer(values, self._box_func)

+    @property


make this a function,

topper-123 · 2017-12-02T21:07:36Z

All green.

jreback

lgtm. ping on green.

jreback · 2017-12-03T15:49:52Z

pandas/tests/indexes/datetimelike.py

+    def test_asobject_deprecated(self):
+        # GH18572
+        d = self.create_index()
+        print(d)


remove this print

jreback · 2017-12-03T15:50:16Z

pandas/tests/indexes/datetimelike.py

+        d = self.create_index()
+        print(d)
+        with tm.assert_produces_warning(FutureWarning):
+            i = d.asobject


you can do the assert inside the with

print here as well

jreback · 2017-12-03T15:51:04Z

pandas/tests/indexes/period/test_ops.py

@@ -290,8 +290,9 @@ def test_comp_nat(self):
                               pd.Period('2011-01-03')])
        right = pd.PeriodIndex([pd.NaT, pd.NaT, pd.Period('2011-01-03')])

-        for l, r in [(left, right), (left.asobject, right.asobject)]:
-            result = l == r
+        for l, r in [(left, right),


can you name these lhs, rhs instead of l, r (to avoid future flake warning)

jreback · 2017-12-03T15:51:27Z

pandas/tests/series/test_dtypes.py

+    def test_asobject_deprecated(self):
+        s = Series(np.random.randn(5), name='foo')
+        with tm.assert_produces_warning(FutureWarning):
+            o = s.asobject


you can assert inside the with

I don't see why: by having it outside, we're clear that the with statement is about the call s.asobject and not the call's return value.

to be honest I actually don't care about the return value here, this is adequately tested elsewhere, the point of this test is just to assert the warning.

pep8speaks · 2017-12-04T09:58:20Z

Hello @topper-123! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on December 04, 2017 at 10:00 Hours UTC

jorisvandenbossche · 2017-12-04T10:49:38Z

Thanks a lot @topper-123 !

topper-123 force-pushed the depr_asobject_II branch 2 times, most recently from dbe3323 to 4cb8776 Compare November 30, 2017 01:46

topper-123 changed the title ~~DEPR: deprecate .asobject method~~ DEPR: deprecate .asobject property Nov 30, 2017

jorisvandenbossche added the Deprecate Functionality to remove in pandas label Nov 30, 2017

jorisvandenbossche reviewed Nov 30, 2017

View reviewed changes

jreback requested changes Nov 30, 2017

View reviewed changes

jreback reviewed Dec 1, 2017

View reviewed changes

topper-123 force-pushed the depr_asobject_II branch from 6e7894a to 656534e Compare December 1, 2017 15:23

jreback requested changes Dec 1, 2017

View reviewed changes

jreback added this to the 0.22.0 milestone Dec 1, 2017

topper-123 force-pushed the depr_asobject_II branch 2 times, most recently from 7c9ccc7 to ad263d9 Compare December 1, 2017 18:22

jreback requested changes Dec 2, 2017

View reviewed changes

topper-123 force-pushed the depr_asobject_II branch 2 times, most recently from f93f999 to a610a0c Compare December 2, 2017 17:52

jreback requested changes Dec 3, 2017

View reviewed changes

tp added 2 commits December 3, 2017 18:30

deprecate .asobject method

9ec47cb

change deprecation of .asobject according to comments

f1ad983

topper-123 force-pushed the depr_asobject_II branch from a610a0c to f1ad983 Compare December 3, 2017 18:30

jorisvandenbossche approved these changes Dec 4, 2017

View reviewed changes

fixup

d61f411

jorisvandenbossche force-pushed the depr_asobject_II branch from 16583a5 to d61f411 Compare December 4, 2017 09:59

jorisvandenbossche merged commit aa5b6e6 into pandas-dev:master Dec 4, 2017

topper-123 deleted the depr_asobject_II branch December 4, 2017 11:23

jreback mentioned this pull request Dec 15, 2017

DEPR: let's deprecate #18262

Closed

34 tasks

DEPR: deprecate .asobject property #18572

DEPR: deprecate .asobject property #18572

Conversation

topper-123 commented Nov 30, 2017 • edited by jreback

codecov bot commented Nov 30, 2017

Codecov Report

codecov bot commented Nov 30, 2017 • edited

Codecov Report

topper-123 commented Nov 30, 2017 • edited

jorisvandenbossche commented Nov 30, 2017

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche Nov 30, 2017 • edited

Choose a reason for hiding this comment

topper-123 commented Nov 30, 2017

jreback commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

topper-123 commented Dec 1, 2017 • edited

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

topper-123 commented Dec 1, 2017

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

topper-123 commented Dec 2, 2017

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Dec 4, 2017 • edited

Comment last updated on December 04, 2017 at 10:00 Hours UTC

jorisvandenbossche commented Dec 4, 2017

topper-123 commented Nov 30, 2017 •

edited by jreback

codecov bot commented Nov 30, 2017 •

edited

topper-123 commented Nov 30, 2017 •

edited

jorisvandenbossche Nov 30, 2017 •

edited

topper-123 commented Dec 1, 2017 •

edited

pep8speaks commented Dec 4, 2017 •

edited