API: Unify .update to generic #23192

h-vetinari · 2018-10-16T22:47:04Z

closes ENH: unify signature for df.update and Series.update #22358 / API/DEPR: replace raise_conflict-kwarg in df.update with errors #23585 / API/BUG: unnecessary upcasting for df.update #23606
tests pass unmodified
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Since #22286 and #21855 have stalled completely, I've decided to go ahead with this independently.

pep8speaks · 2018-10-16T22:47:09Z

Hello @h-vetinari! Thanks for updating the PR.

There are no PEP8 issues in the file pandas/core/frame.py !
There are no PEP8 issues in the file pandas/core/generic.py !
There are no PEP8 issues in the file pandas/core/missing.py !
There are no PEP8 issues in the file pandas/core/panel.py !
There are no PEP8 issues in the file pandas/core/series.py !
There are no PEP8 issues in the file pandas/tests/frame/test_combine_concat.py !
There are no PEP8 issues in the file pandas/tests/series/test_combine_concat.py !
There are no PEP8 issues in the file pandas/tests/test_panel.py !

Comment last updated on November 11, 2018 at 10:14 Hours UTC

jreback · 2018-10-17T01:37:06Z

pandas/core/generic.py

@@ -99,6 +99,32 @@ def _single_replace(self, to_replace, method, inplace, limit):
    return result


+def _update_column(this, that, overwrite=True, filter_func=None,
+                   raise_conflict=False):
+    import pandas.core.computation.expressions as expressions


add a doc-string

jreback · 2018-10-17T01:39:34Z

pandas/core/generic.py

@@ -99,6 +99,32 @@ def _single_replace(self, to_replace, method, inplace, limit):
    return result


+def _update_column(this, that, overwrite=True, filter_func=None,
+                   raise_conflict=False):


move this to pandas/core/missing.py, you can de-privatize it and rename to update_array

jreback · 2018-10-17T01:40:17Z

pandas/core/generic.py

@@ -99,6 +99,32 @@ def _single_replace(self, to_replace, method, inplace, limit):
    return result


+def _update_column(this, that, overwrite=True, filter_func=None,
+                   raise_conflict=False):


I think we have an issue, but pls create one to deprecate this arg and rename to errors= (you can do that in the internal function and put a TODO in the external facing ones).

jreback · 2018-10-17T01:42:29Z

pandas/core/generic.py

+            if updated is None:
+                # don't overwrite Series unnecessarily
+                return
+            self._data._block.values = updated


this is SO unsafe, doesn't handle caching and is reaching into internals. Use self._update_inplace(updated)

if updated is not None: self._update_inplace(updated)

jreback · 2018-10-17T01:43:26Z

pandas/core/series.py

+            original object.
+        overwrite : bool, default True
+            How to handle non-NA values for overlapping keys:
+


can you indicate with a versionadded tag what has changed compared to current

jreback · 2018-10-17T01:47:35Z

pandas/core/generic.py

+                                         filter_func=filter_func,
+                                         raise_conflict=raise_conflict)
+                # don't overwrite columns unnecessarily
+                if updated is None:


if updated is not None: self[col] = updated

jreback · 2018-10-17T01:48:09Z

doc/source/whatsnew/v0.24.0.txt

@@ -200,6 +200,7 @@ Other Enhancements
 - :meth:`Series.resample` and :meth:`DataFrame.resample` have gained the :meth:`Resampler.quantile` (:issue:`15023`).
 - :meth:`Index.to_frame` now supports overriding column name(s) (:issue:`22580`).
 - New attribute :attr:`__git_version__` will return git commit sha of current build (:issue:`21295`).
+- :meth:`Series.update` now supports the same keywords and functionality as :meth:`DataFrame.update` (:issue:`22358`)


can you enumerate what has changed.

jreback · 2018-10-17T01:51:46Z

also have a look at #3025

h-vetinari · 2018-10-17T06:36:07Z

@jreback
Thanks for the review, I'll incorporate it (resp. answer in detail) later. However, this might need a precursor PR to add tests for Series.update - the dtype change for

>>> s = pd.Series([1, 2, 3])
>>> s.update(pd.Series([4, np.nan, 6]))

only got caught by the doctests. Are we fine with changing that?

jreback · 2018-10-17T12:32:07Z

let's make first pass that doesn't change anything, xfail new tests if needed for #23192 (comment).

I would try the approach for #3025 as an alternative

h-vetinari · 2018-11-09T18:52:35Z

Split off the dtype promotion tests for Series (which are already passing on master) off into a precursor: #23604

h-vetinari

@jreback
Incorporated your feedback for deprecating raise_conflict, and fixed the dtype thing as well. PTAL

h-vetinari · 2018-11-09T18:54:26Z

pandas/core/panel.py

+            warnings.simplefilter("ignore", FutureWarning)
+            # do not warn about constructing Panel when reindexing
+            result = super(Panel, self).reindex(**kwargs)
+        return result


I had no intention to add this warning filter here, but otherwise I cannot test the raise_conflict deprecation for Panel, as this warning was throwing me off from trying to catch the deprecation warning. If we don't care about testing that kwarg deprecation for Panel, I can revert this change here.

h-vetinari · 2018-11-09T18:55:12Z

pandas/core/generic.py

+           A    B
+        0  1    4
+        1  2  500
+        2  3    6


This isn't obvious from the diff (due to the code moving files), but now this keeps the dtype - yay!

h-vetinari

Forgot to mention this before.

h-vetinari · 2018-11-09T19:03:51Z

pandas/core/missing.py

+    """
+    updated = _update_array(this, that, overwrite=overwrite,
+                           filter_func=filter_func, errors=errors)
+    return this if updated is None else updated


@jreback
You asked me to move the function here and make it public as update_array. This had the problem that it had the fallback of returning None in case there were no changes, which is necessary to avoid pointlessly updating the columns in NDFrame.update.

However, I was not comfortable with a public function having such unexpected behaviour, so I made the function I used originally private again (but still in core.missing) and just made a thin wrapper around it public, that only filters out the None-returning case.

datapythonista · 2018-11-09T19:53:58Z

@h-vetinari I'm canceling the the old azure builds for this PR as you add new commits (azure auto-cancel is not working at the moment). Shouldn't make a difference to you, as I guess you just care about the last version, but ping me if this causes you any trouble.

jreback

@h-vetinari this PR is again too much going on. pls do this in multiple steps.

h-vetinari · 2018-11-11T17:57:40Z

@jreback
I already split off #23604, but everything else comes directly out of your review. Let's deal with #23604 first though.

h-vetinari · 2018-11-11T18:04:49Z

Though I guess I could split off solving #23606 if I first xfail the tests for #23604 (which represent the current state of Series.update) in this PR

h-vetinari · 2018-11-12T18:16:23Z

Both this PR and #22225 have had persistent a ResourceWarning the last few runs. I first thought it was a flaky thing like those Warnings used to be, but this time, it stayed, and I can reproduce it locally (not with pytest pandas/tests/io/test_parquet.py, but at least with pytest pandas/tests/io).

For example in https://travis-ci.org/pandas-dev/pandas/jobs/453820311 or https://travis-ci.org/pandas-dev/pandas/jobs/453822449:

sys:1: ResourceWarning: unclosed <socket.socket fd=16, family=AddressFamily.AF_INET, type=2050, proto=0, laddr=('0.0.0.0', 0)>
sys:1: ResourceWarning: unclosed <socket.socket fd=15, family=AddressFamily.AF_INET, type=2050, proto=0, laddr=('0.0.0.0', 0)>

and

=============================== warnings summary ===============================
pandas/core/frame.py::pandas.core.frame.DataFrame.to_parquet
  /home/travis/build/pandas-dev/pandas/pandas/io/parquet.py:129: ResourceWarning: unclosed file <_io.BufferedReader name='df.parquet.gzip'>
    **kwargs).to_pandas()

There's also a stderr (or stdout) warning from the parser-tests surfacing somewhere:

..............................................................x...........................................................s....
........................................Skipping line 3: Expected 3 fields in line 3, saw 4
.......................................s.......................................................................................

I've narrowed the ResourceWarning down to the parquet-s3 tests, but couldn't figure out a way to fix it despite many tries, so I'm skipping them for now.

codecov · 2018-11-15T08:02:37Z

Codecov Report

Merging #23192 into master will decrease coverage by <.01%.
The diff coverage is 94.64%.

@@            Coverage Diff             @@
##           master   #23192      +/-   ##
==========================================
- Coverage   92.25%   92.25%   -0.01%     
==========================================
  Files         161      161              
  Lines       51383    51410      +27     
==========================================
+ Hits        47404    47428      +24     
- Misses       3979     3982       +3

Flag	Coverage Δ
#multiple	`90.64% <94.64%> (-0.01%)`	⬇️
#single	`42.3% <10.71%> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/series.py	`93.66% <100%> (-0.02%)`	⬇️
pandas/core/frame.py	`96.99% <100%> (-0.04%)`	⬇️
pandas/core/missing.py	`91.6% <90%> (-0.11%)`	⬇️
pandas/core/generic.py	`96.82% <96.87%> (ø)`	⬆️

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 a23f901...411741a. Read the comment docs.

h-vetinari · 2018-11-26T08:19:07Z

This is still ongoing WIP, but ran into #23823 while trying to use maybe_upcast_putmask here, and then #23833 while trying to fix that method...

jreback · 2019-04-20T17:38:50Z

conceptually this PR is good, but it does too much in a single go, diluting the intent. several much smaller scope PR's would be better.

h-vetinari force-pushed the unify_update branch 3 times, most recently from 16ca125 to c6e9dfb Compare October 16, 2018 23:07

jreback requested changes Oct 17, 2018

View reviewed changes

jreback added Reshaping Concat, Merge/Join, Stack/Unstack, Explode Clean labels Oct 17, 2018

This was referenced Nov 8, 2018

API/DEPR: replace raise_conflict-kwarg in df.update with errors #23585

Closed

TST: add tests for keeping dtype in Series.update #23604

Merged

h-vetinari commented Nov 9, 2018

View reviewed changes

jreback requested changes Nov 11, 2018

View reviewed changes

This was referenced Nov 12, 2018

ENH: Add set_index to Series #22225

Closed

S3 parquet failures on Travis #22934

Closed

API/DEPR: replace "raise_conflict" with "errors" for df.update #23657

Merged

h-vetinari force-pushed the unify_update branch from b1dead0 to accab28 Compare November 13, 2018 07:15

This was referenced Nov 14, 2018

TST: add method/dtype coverage to str-accessor; precursor to #23167 #23582

Merged

BLD/CI: ResourceWarnings are back (sometimes)! #23680

Closed

CLN/CI: Catch that stderr-warning! #23706

Merged

h-vetinari added a commit to h-vetinari/pandas that referenced this pull request Nov 15, 2018

Replicate passing combination of skips from pandas-dev#23192

341c329

API: unify update to generic.py

0bb8325

h-vetinari added 8 commits November 15, 2018 19:30

Fix docstrings

56f569b

Review (jreback)

e1abe77

Add further tests for Series case

802f6ac

Update whatsnew

07784f0

Keep dtype whenever possible; add _update_array; docstring fixes

f120d65

Fix Panel.update

e500a22

Update whatsnew with GH reference

d49b742

Lint, isort, fixes

9183b50

h-vetinari force-pushed the unify_update branch from 073a542 to 9183b50 Compare November 15, 2018 18:40

fix oversight

411741a

This was referenced Nov 16, 2018

CI for boto: fix errors; add coverage; add skip for uncatchable ResourceWarning #23731

Merged

BUG/Internals: maybe_upcast_putmask #23823

Closed

BUG/Internals: maybe_promote #23833

Open

This was referenced Dec 2, 2018

TST: add test coverage for maybe_promote #23982

Merged

ENH: coalesce-method (upgrade for update/combine_first) #22812

Open

h-vetinari mentioned this pull request Feb 24, 2019

REF: Fix maybe_promote #25425

Closed

3 tasks

jreback closed this Apr 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: Unify .update to generic #23192

API: Unify .update to generic #23192

h-vetinari commented Oct 16, 2018 •

edited

Loading

pep8speaks commented Oct 16, 2018 •

edited

Loading

jreback Oct 17, 2018

jreback Oct 17, 2018

jreback Oct 17, 2018

jreback Oct 17, 2018

jreback Oct 17, 2018

jreback Oct 17, 2018

jreback Oct 17, 2018

jreback Oct 17, 2018

jreback commented Oct 17, 2018

h-vetinari commented Oct 17, 2018

jreback commented Oct 17, 2018

h-vetinari commented Nov 9, 2018 •

edited

Loading

h-vetinari left a comment

h-vetinari Nov 9, 2018 •

edited

Loading

h-vetinari Nov 9, 2018 •

edited

Loading

h-vetinari left a comment

h-vetinari Nov 9, 2018

datapythonista commented Nov 9, 2018

jreback left a comment

h-vetinari commented Nov 11, 2018

h-vetinari commented Nov 11, 2018

h-vetinari commented Nov 12, 2018 •

edited

Loading

codecov bot commented Nov 15, 2018 •

edited

Loading

h-vetinari commented Nov 26, 2018

jreback commented Apr 20, 2019

+                         A    B
+1    4
+2  500
+3    6

API: Unify .update to generic #23192

API: Unify .update to generic #23192

Conversation

h-vetinari commented Oct 16, 2018 • edited Loading

pep8speaks commented Oct 16, 2018 • edited Loading

Comment last updated on November 11, 2018 at 10:14 Hours UTC

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

jreback commented Oct 17, 2018

h-vetinari commented Oct 17, 2018

jreback commented Oct 17, 2018

h-vetinari commented Nov 9, 2018 • edited Loading

h-vetinari left a comment

Choose a reason for hiding this comment

h-vetinari Nov 9, 2018 • edited Loading

Choose a reason for hiding this comment

h-vetinari Nov 9, 2018 • edited Loading

Choose a reason for hiding this comment

h-vetinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datapythonista commented Nov 9, 2018

jreback left a comment

Choose a reason for hiding this comment

h-vetinari commented Nov 11, 2018

h-vetinari commented Nov 11, 2018

h-vetinari commented Nov 12, 2018 • edited Loading

codecov bot commented Nov 15, 2018 • edited Loading

Codecov Report

h-vetinari commented Nov 26, 2018

jreback commented Apr 20, 2019

h-vetinari commented Oct 16, 2018 •

edited

Loading

pep8speaks commented Oct 16, 2018 •

edited

Loading

h-vetinari commented Nov 9, 2018 •

edited

Loading

h-vetinari Nov 9, 2018 •

edited

Loading

h-vetinari Nov 9, 2018 •

edited

Loading

h-vetinari commented Nov 12, 2018 •

edited

Loading

codecov bot commented Nov 15, 2018 •

edited

Loading