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

DEPR: deprecate .asobject property #18572

Merged
merged 3 commits into from Dec 4, 2017

Conversation

Projects
None yet
4 participants
@topper-123
Contributor

topper-123 commented Nov 30, 2017

This PR supersedes #18477.
closes #18237

  • [ x] xref #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

This comment has been minimized.

codecov bot commented Nov 30, 2017

Codecov Report

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

Impacted file tree graph

@@            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

This comment has been minimized.

codecov bot commented Nov 30, 2017

Codecov Report

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

Impacted file tree graph

@@            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 topper-123 changed the title from DEPR: deprecate .asobject method to DEPR: deprecate .asobject property Nov 30, 2017

@topper-123

This comment has been minimized.

Contributor

topper-123 commented Nov 30, 2017

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

This comment has been minimized.

Member

jorisvandenbossche commented Nov 30, 2017

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

@@ -420,12 +420,15 @@ def get_values(self):
@property
def asobject(self):
"""
"""DEPRECATED: Use ``astype(object).values`` instead.

This comment has been minimized.

@jorisvandenbossche

jorisvandenbossche Nov 30, 2017

Member

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.

This comment has been minimized.

@jreback

jreback Nov 30, 2017

Contributor

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

This comment has been minimized.

@topper-123

topper-123 Nov 30, 2017

Contributor

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

@@ -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):

This comment has been minimized.

@jorisvandenbossche

jorisvandenbossche Nov 30, 2017

Member

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.

@@ -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`)

This comment has been minimized.

@jreback

jreback Nov 30, 2017

Contributor

no need to suggest .values

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

This comment has been minimized.

@jreback

jreback Nov 30, 2017

Contributor

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

This comment has been minimized.

@jorisvandenbossche

jorisvandenbossche Nov 30, 2017

Member

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.

This comment has been minimized.

@jreback

jreback Nov 30, 2017

Contributor

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.

This comment has been minimized.

@jorisvandenbossche

jorisvandenbossche Nov 30, 2017

Member

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

This comment has been minimized.

Contributor

topper-123 commented Nov 30, 2017

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

This comment has been minimized.

Contributor

jreback commented Dec 1, 2017

@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.

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

This comment has been minimized.

@jreback

jreback Dec 1, 2017

Contributor

see what effect of removing this does

This comment has been minimized.

@topper-123

topper-123 Dec 1, 2017

Contributor

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

This comment has been minimized.

Contributor

topper-123 commented Dec 1, 2017

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.

@@ -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)

This comment has been minimized.

@jreback

jreback Dec 1, 2017

Contributor

add the issue number as a comment

use d = self.create_index()

This comment has been minimized.

@topper-123

topper-123 Dec 1, 2017

Contributor

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

@jreback jreback added this to the 0.22.0 milestone Dec 1, 2017

@topper-123

This comment has been minimized.

Contributor

topper-123 commented Dec 1, 2017

All green. Please verify if last change is ok.

@jreback

needs a rebase and a change. otherwise lgtm.

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

This comment has been minimized.

@jreback

jreback Dec 2, 2017

Contributor

make this a function,

@topper-123

This comment has been minimized.

Contributor

topper-123 commented Dec 2, 2017

All green.

@jreback

lgtm. ping on green.

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

This comment has been minimized.

@jreback

jreback Dec 3, 2017

Contributor

remove this print

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

This comment has been minimized.

@jreback

jreback Dec 3, 2017

Contributor

you can do the assert inside the with

This comment has been minimized.

@jreback

jreback Dec 3, 2017

Contributor

print here as well

@@ -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),

This comment has been minimized.

@jreback

jreback Dec 3, 2017

Contributor

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

This comment has been minimized.

@topper-123

topper-123 Dec 3, 2017

Contributor

ok.

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

This comment has been minimized.

@jreback

jreback Dec 3, 2017

Contributor

you can assert inside the with

This comment has been minimized.

@topper-123

topper-123 Dec 3, 2017

Contributor

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.

This comment has been minimized.

@jreback

jreback Dec 3, 2017

Contributor

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

This comment has been minimized.

pep8speaks commented Dec 4, 2017

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 jorisvandenbossche merged commit aa5b6e6 into pandas-dev:master Dec 4, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jorisvandenbossche

This comment has been minimized.

Member

jorisvandenbossche commented Dec 4, 2017

Thanks a lot @topper-123 !

@topper-123 topper-123 deleted the topper-123:depr_asobject_II branch Dec 4, 2017

@jreback jreback referenced this pull request Dec 15, 2017

Open

DEPR: let's deprecate #18262

12 of 36 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment