DEPR: deprecate .asobject property by topper-123 · Pull Request #18572 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation38 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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.
DEPR: deprecate .asobject method DEPR: deprecate .asobject property
all is green, I think the failure on appveyoyor is unrelated to the PR.
Some points:
- We discussed that
asobjectshould just returnself.astype(object). It turns out that the relevant versions ofindex .astypecall.asobjectthemselves, so to avoid circular calls, I've kept a new internal._asobject. This also avoids code duplication in the variousastypemethods. All internal calls use.astype(object)and not.asobject. So._asobjectis only used byastype. - The return values of the various
Index.asobjectisastype(object), but forSeries.asobjectthe return value isastype(object).values. so there's a need to differentiate between the two.
In general: please don't open new PRs, but just update the original one.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't add .values, that is just adding even more confusion.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe test_astype_object ? (if it is only testing that)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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`) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to suggest .values
| @@ -421,15 +421,25 @@ def _isnan(self): |
|---|
| return (self.asi8 == iNaT) |
| @property |
| def asobject(self): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually want to completely remove _asobject, this just adds even more technical debt.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
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.
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 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see what effect of removing this does
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two tests fail, both at pandas\tests\test_categorical.py:811 (parameterized tests, of categorical of dtype='timedelta64[h]' and is_ordered=True).
Updated.
asobject is a property. I therefore have updated _deprecations to include it. Otherwise the warning get printed in ipython everytime we tab complete.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add the issue number as a comment
use d = self.create_index()
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, uploaded. self.create_index() takes care of calling the correct index type?
All green. Please verify if last change is ok.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make this a function,
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. ping on green.
| def test_asobject_deprecated(self): |
|---|
| # GH18572 |
| d = self.create_index() |
| print(d) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this print
| d = self.create_index() |
|---|
| print(d) |
| with tm.assert_produces_warning(FutureWarning): |
| i = d.asobject |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can do the assert inside the with
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you name these lhs, rhs instead of l, r (to avoid future flake warning)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok.
| def test_asobject_deprecated(self): |
|---|
| s = Series(np.random.randn(5), name='foo') |
| with tm.assert_produces_warning(FutureWarning): |
| o = s.asobject |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can assert inside the with
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
tp added 2 commits
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
