BUG: Fixing DataFrame.Update crashes when NaT present #49395

wxing11 · 2022-10-30T05:48:29Z

closes DataFrame.update crashes with overwrite=False when NaT present #16713 (Replace xxxx with the GitHub issue number)
Tests added and passed if fixing a bug or adding a new feature
All code checks passed.
Added type annotations to new arguments/methods/functions.
Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

mroeschke · 2022-10-31T18:01:32Z

pandas/core/frame.py

 this = self[col]._values
 that = other[col]._values
+
+ if all(isna(that)):


From the original issue, it suggests that this may be an issue in the reindex_like operations being incompatible with something, so I don't think a patched-over check like this is appropriate

Hi! Thanks for reviewing!

This was my assessment of it in the original issue: #16713 (comment)

Based on that, I think the issue in reindex_like is just that when it creates columns that aren't in the pre-reindex dataframe that they aren't datetime/datetime compatible by default. Is that the part I should be looking to change instead?

Is that the part I should be looking to change instead?

Ideally yes, or possibly when the masks are compared.

Will look into this more, thanks for the guidance!

pandas/core/frame.py

mroeschke · 2022-11-07T19:36:02Z

pandas/core/frame.py

+ other = other.reindex(self.index)

- for col in self.columns:
+ shared_cols = self.columns.intersection(other.columns)


@MarcoGorelli I know this solution kinda side-steps the original issue of null-matching (and reindex introducing an entire NA-column that doesn't need updating I think), but happy to have your thoughts on this solution.

I think this is fine

Something that comes to mind is that it'll still error if other contains a column all of nan, e.g.:

In [1]: df1 = pd.DataFrame({'a': [NaT]}) In [2]: df2 = pd.DataFrame({'a': [np.nan]}) In [3]: df1.update(df2, overwrite=False) --------------------------------------------------------------------------- TypeError Traceback (most recent call last) Cell In [3], line 1 ----> 1 df1.update(df2, overwrite=False) File ~/pandas-dev/pandas/core/frame.py:8096, in DataFrame.update(self, other, join, overwrite, filter_func, errors) 8093 if mask.all(): 8094 continue -> 8096 self.loc[:, col] = expressions.where(mask, this, that) File ~/pandas-dev/pandas/core/computation/expressions.py:258, in where(cond, a, b, use_numexpr) 246 """ 247 Evaluate the where condition cond on a and b. 248 (...) 255 Whether to try to use numexpr. 256 """ 257 assert _where is not None --> 258 return _where(cond, a, b) if use_numexpr else _where_standard(cond, a, b) File ~/pandas-dev/pandas/core/computation/expressions.py:188, in _where_numexpr(cond, a, b) 181 result = ne.evaluate( 182 "where(cond_value, a_value, b_value)", 183 local_dict={"cond_value": cond, "a_value": a, "b_value": b}, 184 casting="safe", 185 ) 187 if result is None: --> 188 result = _where_standard(cond, a, b) 190 return result File ~/pandas-dev/pandas/core/computation/expressions.py:172, in _where_standard(cond, a, b) 170 def _where_standard(cond, a, b): 171 # Caller is responsible for extracting ndarray if necessary --> 172 return np.where(cond, a, b) File <__array_function__ internals>:180, in where(*args, **kwargs) TypeError: The DType <class 'numpy.dtype[datetime64]'> could not be promoted by <class 'numpy.dtype[float64]'>. This means that no common DType exists for the given inputs. For example they cannot be stored in a single array unless the dtype is `object`. The full list of DTypes is: (<class 'numpy.dtype[datetime64]'>, <class 'numpy.dtype[float64]'>)

but arguably that's desirable behaviour - you wouldn't want to update with a column of an incompatible dtype, regardless of whether its values were all missing or not.

And value-dependent behaviour wouldn't be great, so I like this solution more than the originally-suggested "if all nan then skip"

MarcoGorelli

The solution looks fine to me, I like this more than the originally-suggested one

This'll need a whatsnew note too

MarcoGorelli · 2022-11-08T14:49:48Z

pandas/core/frame.py

+ other = other.reindex(self.index)

- for col in self.columns:
+ shared_cols = self.columns.intersection(other.columns)


I think this is fine

Something that comes to mind is that it'll still error if other contains a column all of nan, e.g.:

In [1]: df1 = pd.DataFrame({'a': [NaT]}) In [2]: df2 = pd.DataFrame({'a': [np.nan]}) In [3]: df1.update(df2, overwrite=False) --------------------------------------------------------------------------- TypeError Traceback (most recent call last) Cell In [3], line 1 ----> 1 df1.update(df2, overwrite=False) File ~/pandas-dev/pandas/core/frame.py:8096, in DataFrame.update(self, other, join, overwrite, filter_func, errors) 8093 if mask.all(): 8094 continue -> 8096 self.loc[:, col] = expressions.where(mask, this, that) File ~/pandas-dev/pandas/core/computation/expressions.py:258, in where(cond, a, b, use_numexpr) 246 """ 247 Evaluate the where condition cond on a and b. 248 (...) 255 Whether to try to use numexpr. 256 """ 257 assert _where is not None --> 258 return _where(cond, a, b) if use_numexpr else _where_standard(cond, a, b) File ~/pandas-dev/pandas/core/computation/expressions.py:188, in _where_numexpr(cond, a, b) 181 result = ne.evaluate( 182 "where(cond_value, a_value, b_value)", 183 local_dict={"cond_value": cond, "a_value": a, "b_value": b}, 184 casting="safe", 185 ) 187 if result is None: --> 188 result = _where_standard(cond, a, b) 190 return result File ~/pandas-dev/pandas/core/computation/expressions.py:172, in _where_standard(cond, a, b) 170 def _where_standard(cond, a, b): 171 # Caller is responsible for extracting ndarray if necessary --> 172 return np.where(cond, a, b) File <__array_function__ internals>:180, in where(*args, **kwargs) TypeError: The DType <class 'numpy.dtype[datetime64]'> could not be promoted by <class 'numpy.dtype[float64]'>. This means that no common DType exists for the given inputs. For example they cannot be stored in a single array unless the dtype is `object`. The full list of DTypes is: (<class 'numpy.dtype[datetime64]'>, <class 'numpy.dtype[float64]'>)

but arguably that's desirable behaviour - you wouldn't want to update with a column of an incompatible dtype, regardless of whether its values were all missing or not.

And value-dependent behaviour wouldn't be great, so I like this solution more than the originally-suggested "if all nan then skip"

MarcoGorelli · 2022-11-08T14:50:24Z

pandas/core/frame.py

- for col in self.columns:
+ shared_cols = self.columns.intersection(other.columns)
+
+ for col in shared_cols:


let's keep it on one line

for col in self.columns.intersection(other.columns):

MarcoGorelli · 2022-11-08T14:50:42Z

pandas/core/frame.py

 other = DataFrame(other)

- other = other.reindex_like(self)
+ # reindex rows, non-matching columns get skipped


not sure we need this comment

MarcoGorelli · 2022-11-08T14:51:25Z

pandas/tests/frame/methods/test_update.py

+ "B": [
+ pd.NaT,
+ pd.to_datetime("2016-01-01"),
+ ],


can we keep this on a single line?

pandas/tests/frame/methods/test_update.py

MarcoGorelli · 2022-11-08T14:53:24Z

pandas/tests/frame/methods/test_update.py

+ }
+ )
+ df2 = DataFrame({"A": [2, 3]})
+


let's remove all these newlines in the tests

MarcoGorelli · 2022-11-08T14:56:35Z

pandas/tests/frame/methods/test_update.py

+
+ tm.assert_frame_equal(df, expected)
+
+ def test_update_dt_column_with_NaT_create_row(self):


not really sure what this test adds, I'd suggest to either:

parametrize over the first test

just remove this one

wxing11 · 2022-11-09T01:10:59Z

@MarcoGorelli Made changes for the comments you left and added to the whatsnew!

MarcoGorelli

The change looks good to me (@mroeschke any further comments?), I just have a small comment on the whatsnew note

MarcoGorelli · 2022-11-12T11:48:50Z

doc/source/whatsnew/v2.0.0.rst

 - Bug in :meth:`Index.equals` raising ``TypeError`` when :class:`Index` consists of tuples that contain ``NA`` (:issue:`48446`)
 - Bug in :meth:`Series.map` caused incorrect result when data has NaNs and defaultdict mapping was used (:issue:`48813`)
 - Bug in :class:`NA` raising a ``TypeError`` instead of return :class:`NA` when performing a binary operation with a ``bytes`` object (:issue:`49108`)
+- Bug in :meth:`Dataframe.update` raising ``TypeError`` when column has NaT values (:issue:`16713`)


Could we clarify please - if I understand correctly, it was only raising a TypeError if the column had NaT values and it wasn't present in the other DataFrame and you were using overwrite=False

Also, if you write NaT, make sure to put double ticks around it, like this

``NaT``

Updated the message, let me know if the new one works!

mroeschke

Changes look fine to me. Merge when all ready @MarcoGorelli

MarcoGorelli

Making a minor update to docs, hope it's OK, but rest looks good, thanks @wxing11 !

doc/source/whatsnew/v2.0.0.rst

* Fixes DataFrame.update crashes when NaT present. GH16713 * Formatting * No longer reindexing columns, skipping non-matching columns * switching to using set intersection to find shared columns * switching to using index intersection * Removing unnecessary variable creation * Formatting and removing test * Adding to whatsnew * updating whatsnew message * Update doc/source/whatsnew/v2.0.0.rst Co-authored-by: Marco Edward Gorelli <33491632+MarcoGorelli@users.noreply.github.com>

wxing11 added 2 commits October 30, 2022 05:26

Fixes DataFrame.update crashes when NaT present. GH16713

2d126dd

Formatting

b4c792a

mroeschke reviewed Oct 31, 2022

View reviewed changes

mroeschke added the DataFrame DataFrame data structure label Oct 31, 2022

MarcoGorelli mentioned this pull request Nov 1, 2022

DataFrame.update crashes with overwrite=False when NaT present #16713

Closed

No longer reindexing columns, skipping non-matching columns

c63d1cc

mroeschke reviewed Nov 3, 2022

View reviewed changes

pandas/core/frame.py Outdated Show resolved Hide resolved

wxing11 added 2 commits November 3, 2022 17:59

switching to using set intersection to find shared columns

7bea232

switching to using index intersection

9ee4329

mroeschke reviewed Nov 7, 2022

View reviewed changes

MarcoGorelli suggested changes Nov 8, 2022

View reviewed changes

wxing11 added 3 commits November 8, 2022 22:41

Removing unnecessary variable creation

f191629

Formatting and removing test

35fa0a8

Adding to whatsnew

ead244a

MarcoGorelli self-requested a review November 9, 2022 09:11

MarcoGorelli suggested changes Nov 12, 2022

View reviewed changes

updating whatsnew message

bd07c5a

mroeschke reviewed Nov 14, 2022

View reviewed changes

Merge branch 'main' into fixing-dataframe-update-crashes

8a1e0f7

MarcoGorelli approved these changes Nov 15, 2022

View reviewed changes

doc/source/whatsnew/v2.0.0.rst Outdated Show resolved Hide resolved

Update doc/source/whatsnew/v2.0.0.rst

d8167ea

MarcoGorelli added this to the 2.0 milestone Nov 15, 2022

MarcoGorelli merged commit 12ff4f4 into pandas-dev:main Nov 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

BUG: Fixing DataFrame.Update crashes when NaT present #49395

BUG: Fixing DataFrame.Update crashes when NaT present #49395

Uh oh!

wxing11 commented Oct 30, 2022 •

edited

Loading

mroeschke Oct 31, 2022

wxing11 Oct 31, 2022

mroeschke Oct 31, 2022

wxing11 Oct 31, 2022

Uh oh!

mroeschke Nov 7, 2022

MarcoGorelli Nov 8, 2022

MarcoGorelli left a comment

MarcoGorelli Nov 8, 2022

MarcoGorelli Nov 8, 2022

MarcoGorelli Nov 8, 2022

MarcoGorelli Nov 8, 2022

Uh oh!

MarcoGorelli Nov 8, 2022

MarcoGorelli Nov 8, 2022

wxing11 commented Nov 9, 2022

MarcoGorelli left a comment

MarcoGorelli Nov 12, 2022

wxing11 Nov 12, 2022

mroeschke left a comment

MarcoGorelli left a comment

Uh oh!

Labels

3 participants


		tm.assert_frame_equal(df, expected)

		def test_update_dt_column_with_NaT_create_row(self):

Uh oh!

BUG: Fixing DataFrame.Update crashes when NaT present #49395

BUG: Fixing DataFrame.Update crashes when NaT present #49395

Uh oh!

Conversation

wxing11 commented Oct 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoGorelli left a 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

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wxing11 commented Nov 9, 2022

MarcoGorelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mroeschke left a comment

Choose a reason for hiding this comment

MarcoGorelli left a comment

Choose a reason for hiding this comment

Uh oh!

Labels

3 participants

wxing11 commented Oct 30, 2022 •

edited

Loading