DEPR: Series/DataFrame.append (#35407) #44539
Conversation
jreback
left a comment
jreback
left a comment
There was a problem hiding this comment.
ok to filter warnings and have a single test to assert the deprecation warning
neinkeinkaffee
force-pushed
the
35407-deprecate-append
branch
from
356579d to
2b818bb
Compare
|
Thanks for the suggestions. I've tried to address them. Could you re-review and let me know your feedback? Cheers! |
phofl
left a comment
phofl
left a comment
There was a problem hiding this comment.
Web and docs is failing because of the warnings
|
Question on the failing web and docs: looking at the failing doc build, some of the cases are due to direct calls to append, which I can change into concat. But others are indirect, e.g. |
|
no all usages should be changes in tests and docs except those we r explicitly doing |
|
@jreback Just to make sure I don't misunderstand: given that frame.memory_usage is neither in doc nor test, I shouldn't change the call to series.append to concat there, but instead add a :okwarning: to the ipython codeblock? And likewise not change any other instances were append is called in the pandas code that's neither documentation nor test? |
|
no u do not add okwarning anywhere u need to fix this for example in the code not to output a warning (as this is tested in many tests) |
|
when you push pls merge master |
|
Yes, will do! |
| msg = "Index has duplicates." | ||
| with pytest.raises(pd.errors.DuplicateLabelError, match=msg): | ||
| func(s) | ||
| pd.concat([pd.Series(0, index=["a", "b"]), s]) |
There was a problem hiding this comment.
can you construct the series outside of the pytest.raises; makes clearer exactly what part of this line is raising
| df = orig.copy() | ||
| df.loc["a", :] = df.iloc[0] | ||
| exp = orig._append(Series(df.iloc[0], name="a")) | ||
| s = Series(df.iloc[0], name="a") |
|
|
||
| # append | ||
| result = df.iloc[0:8, :].append(df.iloc[8:]) | ||
| result = concat([df.iloc[0:8, :], df.iloc[8:]]) |
There was a problem hiding this comment.
are these redundant with the tests above? this may be better thought of as a targeted test for _append.
pandas/core/frame.py
Outdated
| self.index.memory_usage(deep=deep), index=["Index"] | ||
| ).append(result) | ||
| ) | ||
| result = concat([index_memory_usage, result]) |
There was a problem hiding this comment.
i'd rather keep using _append internally; should be more performant and avoids circular dependency
| with tm.assert_produces_warning(None): | ||
| # GH#35407 | ||
| with tm.assert_produces_warning(FutureWarning): | ||
| result = df1.append(df2, sort=sort) |
There was a problem hiding this comment.
if this file is about testing 'append', then these should just be changed to test '_append'
|
@neinkeinkaffee can you merge master |
jreback
left a comment
jreback
left a comment
There was a problem hiding this comment.
minor comment, ping on green
doc/source/user_guide/merging.rst
Outdated
| While not especially efficient (since a new object must be created), you can | ||
| append a single row to a ``DataFrame`` by passing a ``Series`` or dict to | ||
| ``append``, which returns a new ``DataFrame`` as above. | ||
| You can append a single row in form of a series to a ``DataFrame`` in-place |
There was a problem hiding this comment.
don't show L416, just make the alternative the thing
|
closes #22957? |
|
@jreback I've changed the docs and removed the part about adding rows in-place. However, the Database / Linux_py38_IO pipeline currently fails with |
|
@neinkeinkaffee if you can merge master again; just want to get a clean run here. we do have some occasional test failures on CI (though some recent PRs do address some of these) |
mroeschke
left a comment
mroeschke
left a comment
There was a problem hiding this comment.
LGTM. Great job @neinkeinkaffee
|
thanks @neinkeinkaffee yep very nice! |
6 participants
DataFrame.appendbehave related to indexes types? #22957