BUG: handle nan values in DataFrame.update when overwrite=False (#15593) #16430

pcluo · 2017-05-22T19:38:49Z

BUG: handle nan values in DataFrame.update when overwrite=False (#15593)

add nan test for DataFrame.update

update whatsnew v0.20.2

closes handle nan values in DataFrame.update when overwrite=False #15593
closes handle nan values in DataFrame.update when overwrite=False #15593 #15617
tests added / passed
passes git diff upstream/master --name-only -- '*.py' | flake8 --diff
whatsnew entry

TomAugspurger · 2017-05-22T19:53:02Z

doc/source/whatsnew/v0.20.2.txt

@@ -37,6 +37,7 @@ Bug Fixes
 ~~~~~~~~~

 - Bug in using ``pathlib.Path`` or ``py.path.local`` objects with io functions (:issue:`16291`)
+- Bug in using ``DataFrame.update`` when overwrite=False and the second value is nan


Could you add a ref to the issue ath the end here

(:issue:`15593`)

TomAugspurger · 2017-05-22T19:56:59Z

pandas/tests/frame/test_combine_concat.py

+        df1 = DataFrame({'A': [1, None, 3], 'B': date_range('2000', periods=3)})
+        df2 = DataFrame({'A': [None, 2, 3]})
+        df1.update(df2, overwrite=False)
+        # it works!


could you make an exected dataframe and compare?

expected = df1 = DataFrame({'A': [1.0, 2, 3], 'B': date_range('2000', periods=3)}) tm.assert_frame_equal(df1, expected)

pcluo · 2017-05-22T20:31:57Z

yes, i just made the changes.

…

On Mon, May 22, 2017 at 3:58 PM, Tom Augspurger ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In doc/source/whatsnew/v0.20.2.txt <#16430 (comment)>: > @@ -37,6 +37,7 @@ Bug Fixes ~~~~~~~~~ - Bug in using ``pathlib.Path`` or ``py.path.local`` objects with io functions (:issue:`16291`) +- Bug in using ``DataFrame.update`` when overwrite=False and the second value is nan Could you add a ref to the issue ath the end here (:issue:`15593`) ------------------------------ In pandas/tests/frame/test_combine_concat.py <#16430 (comment)>: > @@ -763,3 +763,13 @@ def test_concat_datetime_datetime64_frame(self): # it works! pd.concat([df1, df2_obj]) + + +class TestDataFrameUpdate(TestData): + + def test_update_nan(self): + # #15593 #15617 + df1 = DataFrame({'A': [1, None, 3], 'B': date_range('2000', periods=3)}) + df2 = DataFrame({'A': [None, 2, 3]}) + df1.update(df2, overwrite=False) + # it works! could you make an exected dataframe and compare? expected = df1 = DataFrame({'A': [1.0, 2, 3], 'B': date_range('2000', periods=3)}) tm.assert_frame_equal(df1, expected) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#16430 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABSVsjx9erVVSzQBeyQlFJlbjjRXyqd3ks5r8ejbgaJpZM4Niywe> .

TomAugspurger · 2017-05-22T21:09:37Z

pandas/tests/frame/test_combine_concat.py

+        df2 = DataFrame({'A': [None, 2, 3]})
+        df1.update(df2, overwrite=False)
+        # it works!
+        tm.assert_frame_equal(df1, expected)


Can you add a second test with

df1 = DataFrame({'A': [1,None,3], 'B': date_range('2000', periods=3)}) df1.update(df2, overwrite=False)

just to make sure we cover the original issue? You can do it in this same test, and use the same expected

jreback · 2017-05-22T22:42:34Z

pandas/tests/frame/test_combine_concat.py

+
+    def test_update_nan(self):
+        # #15593 #15617
+        expected = df1 = DataFrame({'A': [1.0, 2, 3], 'B': date_range('2000', periods=3)})


do like this

df1 = DataFrame(....) df2 = DataFrame(....) expected = df1 result = df1.update(....) tm.assert_frame_equal(result, expected)

I think update is inplace? So expected = df1.copy()

no it returns a new object (but has inplace arg)

sorry, you are right @TomAugspurger . Hmm that is pretty bogus it should have a kwarg for this.

jreback · 2017-05-22T22:43:56Z

doc/source/whatsnew/v0.20.2.txt

@@ -37,6 +37,7 @@ Bug Fixes
 ~~~~~~~~~

 - Bug in using ``pathlib.Path`` or ``py.path.local`` objects with io functions (:issue:`16291`)
+- Bug in using ``DataFrame.update`` when overwrite=False and the second value is nan (:issue:`15593`)


add the 2nd issue as well.

Bug in DataFrame.update() with overwrite=False and NaN values

TomAugspurger · 2017-05-23T19:31:55Z

@pcluo can you update with the requested changes? Thanks!

pcluo · 2017-05-24T00:24:18Z

@TomAugspurger yes, plz check.

TomAugspurger · 2017-05-24T01:30:26Z

pandas/tests/frame/test_combine_concat.py

+        tm.assert_frame_equal(df1, expected)
+
+        # test 2
+        expected = df1 = DataFrame({'A': [1, None, 3], 'B': date_range('2000', periods=3)})


Sorry, I think this got lost in the conversation, but can you write this as:

df1 = DataFrame({'A': [1, None, 3], 'B': date_range('2000', periods=3)}) df2 = DataFrame({'A': [None, 2, 3]}) expected = DataFrame({'A': [1, 2, 3], 'B': date_range('2000', periods=3)})

The one on line 773 can be a bit shorter, just

df1 = DataFrame({'A': [1.0, 2, 3], 'B': date_range('2000', periods=3)}) expected = df1.copy()

basically, since .update works inplace, the test will always be true no matter what happens. expected and df1 are reference to the same object. It's like

In [2]: a = b = {'x': 1} In [3]: a['y'] = 2 In [4]: a == b Out[4]: True

Which "shouldn't" be true (but is, since a and b are both references to the same object). Let me know if that makes sense.

pcluo · 2017-05-24T06:25:19Z

@TomAugspurger just made another attempt at this. Sorry the last conversation got lost when i squashed commits. I changed [1, 2, 3] in test 2 to [1.0, 2, 3] otherwise the dtypes are not equal, float64 vs int64.

jreback · 2017-05-24T10:39:06Z

can you rebase on master

TomAugspurger · 2017-05-24T13:25:06Z

I changed [1, 2, 3] in test 2 to [1.0, 2, 3] otherwise the dtypes are not equal, float64 vs int64.

Yeah, that sounds right.

If you rebase and force push, all the tests should pass.

git fetch upstream
git rebase upstream/master
git push -f origin master

FYI, typically you want to checkout a new branch for features / bug fixes. http://pandas.pydata.org/pandas-docs/stable/contributing.html#creating-a-branch Not a huge deal though.

…as-dev#15593) add nan test for DataFrame.update update whatsnew v0.20.2

pcluo · 2017-05-24T18:20:46Z

it seems all tests passed now. thx @TomAugspurger and will checkout a new branch next time.

TomAugspurger · 2017-05-24T18:33:29Z

Looks like Travis isn't picking up your commits for some reason. I'll ask their support.

jreback · 2017-05-24T23:15:35Z

thanks!

TomAugspurger · 2017-05-25T12:23:56Z

FYI @pcluo TravisCI support thought that if you sign into travis with your github account then things should be good.

…as-dev#15593) (pandas-dev#16430) (cherry picked from commit 85080aa)

…) (#16430) (cherry picked from commit 85080aa)

…as-dev#15593) (pandas-dev#16430)

pcluo mentioned this pull request May 22, 2017

handle nan values in DataFrame.update when overwrite=False #15593 #15617

Closed

TomAugspurger added this to the 0.20.2 milestone May 22, 2017

TomAugspurger added Dtype Conversions Unexpected or buggy dtype conversions Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels May 22, 2017

TomAugspurger reviewed May 22, 2017

View reviewed changes

pcluo force-pushed the master branch from ee999f7 to e6b11bd Compare May 22, 2017 20:29

TomAugspurger reviewed May 22, 2017

View reviewed changes

jreback requested changes May 22, 2017

View reviewed changes

pcluo force-pushed the master branch from e6b11bd to c832fdd Compare May 24, 2017 00:23

TomAugspurger reviewed May 24, 2017

View reviewed changes

pcluo force-pushed the master branch 2 times, most recently from d8bde84 to be7a230 Compare May 24, 2017 06:20

pcluo closed this May 24, 2017

pcluo reopened this May 24, 2017

BUG: handle nan values in DataFrame.update when overwrite=False (pand…

468a798

…as-dev#15593) add nan test for DataFrame.update update whatsnew v0.20.2

pcluo force-pushed the master branch from be7a230 to 468a798 Compare May 24, 2017 17:05

Restart TravisCI

d850508

jreback approved these changes May 24, 2017

View reviewed changes

jreback merged commit 85080aa into pandas-dev:master May 24, 2017

TomAugspurger pushed a commit to TomAugspurger/pandas that referenced this pull request May 29, 2017

BUG: handle nan values in DataFrame.update when overwrite=False (pand…

e735629

…as-dev#15593) (pandas-dev#16430) (cherry picked from commit 85080aa)

TomAugspurger added Backported and removed Needs Backport labels May 30, 2017

TomAugspurger pushed a commit that referenced this pull request May 30, 2017

BUG: handle nan values in DataFrame.update when overwrite=False (#15593…

64655e7

…) (#16430) (cherry picked from commit 85080aa)

stangirala pushed a commit to stangirala/pandas that referenced this pull request Jun 11, 2017

BUG: handle nan values in DataFrame.update when overwrite=False (pand…

87dd46d

…as-dev#15593) (pandas-dev#16430)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: handle nan values in DataFrame.update when overwrite=False (#15593) #16430

BUG: handle nan values in DataFrame.update when overwrite=False (#15593) #16430

pcluo commented May 22, 2017 •

edited by jreback

Loading

TomAugspurger May 22, 2017

TomAugspurger May 22, 2017

pcluo commented May 22, 2017 via email

TomAugspurger May 22, 2017

jreback May 22, 2017

TomAugspurger May 23, 2017

jreback May 23, 2017

jreback May 23, 2017

jreback May 23, 2017

jreback May 22, 2017

TomAugspurger commented May 23, 2017

pcluo commented May 24, 2017

TomAugspurger May 24, 2017 •

edited

Loading

pcluo commented May 24, 2017

jreback commented May 24, 2017

TomAugspurger commented May 24, 2017

pcluo commented May 24, 2017

TomAugspurger commented May 24, 2017

jreback commented May 24, 2017

TomAugspurger commented May 25, 2017

BUG: handle nan values in DataFrame.update when overwrite=False (#15593) #16430

BUG: handle nan values in DataFrame.update when overwrite=False (#15593) #16430

Conversation

pcluo commented May 22, 2017 • edited by jreback Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcluo commented May 22, 2017 via email

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

TomAugspurger commented May 23, 2017

pcluo commented May 24, 2017

TomAugspurger May 24, 2017 • edited Loading

Choose a reason for hiding this comment

pcluo commented May 24, 2017

jreback commented May 24, 2017

TomAugspurger commented May 24, 2017

pcluo commented May 24, 2017

TomAugspurger commented May 24, 2017

jreback commented May 24, 2017

TomAugspurger commented May 25, 2017

pcluo commented May 22, 2017 •

edited by jreback

Loading

TomAugspurger May 24, 2017 •

edited

Loading