BUG: during interchanging from non-pandas tz-aware data #54287

natmokval · 2023-07-28T15:55:48Z

Added a test for interchanging from non-pandas tz-aware.

EDIT: fixed bug which was raising during interchanging from non-pandas tz-aware data.

MarcoGorelli · 2023-07-28T15:58:36Z

pandas/tests/interchange/test_impl.py

+
+    arr = pa.array([datetime(2020, 1, 1), None, datetime(2020, 1, 2)])
+    arr = pc.assume_timezone(arr, "Asia/Kathmandu")
+    result = pa.table({"arr": arr}).to_pandas()


wait we need to go via the interchange protocol - could you use result = from_dataframe(pa.table(...))?

oops, my mistake. I'll do that via the interchange protocol.

MarcoGorelli · 2023-07-28T16:19:55Z

pandas/tests/interchange/test_impl.py

+
+    arr = pa.array([datetime(2020, 1, 1), None, datetime(2020, 1, 2)])
+    arr = pc.assume_timezone(arr, "Asia/Kathmandu")
+    result = from_dataframe(pa.table({"arr": arr}).to_pandas())


the interchange protocol means we can avoid to_pandas calls - so long as __dataframe__ is available (which it is for pyarrow tables) we can just do:

Suggested change

result = from_dataframe(pa.table({"arr": arr}).to_pandas())

result = from_dataframe(pa.table({"arr": arr}))

strange, it doesn't work for me. If I chage the line 307 to result = from_dataframe(pa.table({"arr": arr})), I get an error

pandas.errors.SettingWithCopyError: modifications to a method of a datetimelike object are not supported and are discarded. Change values on the original.

thanks - that looks like a warning which should be suppressed somewhere

could you please:

in this test, just add with tm.assert_produces_warning to silence it and get the test to pass

open a new issue about how the snippet above produces a warning?

Okay. I'll do that.

seems I can’t silence it, because it’s an error. Looks like it comes from the method _delegate_method. I corrected the test, now it checks this error.

mroeschke · 2023-07-28T17:32:46Z

pandas/tests/interchange/test_impl.py

+
+def test_interchange_from_non_pandas_tz_aware():
+    # GH 54239
+    import pyarrow as pa


Suggested change

import pyarrow as pa

pa = pytest.importorskip("pyarrow")

thanks, I tried it, but still have the same error.

This will help CI builds that don't have pyarrow installed

ahh, that is the reason why many checks failed

MarcoGorelli · 2023-07-31T08:29:35Z

pandas/tests/interchange/test_impl.py

+    exchange_df = table.__dataframe__()
+
+    msg = "modifications to a method of a datetimelike object are not supported."
+    with pytest.raises(SettingWithCopyError, match=msg):


I don't think this should be raising an error though

In

try: data[null_pos] = None except TypeError: # TypeError happens if the `data` dtype appears to be non-nullable # in numpy notation (bool, int, uint). If this happens, # cast the `data` to nullable float dtype. data = data.astype(float) data[null_pos] = None

, could we add an extra except, which catches SettingWithCopyError, and in that case makes a copy of data (data = data.copy()) and then sets the nulls (data[null_pos] = None)?

Thank you, I added the extra except, it works very well. I updated my test and added a line to whatsnew/v2.1.0.rst as well.

MarcoGorelli · 2023-07-31T16:02:48Z

pandas/tests/interchange/test_impl.py

+
+def test_interchange_from_non_pandas_tz_aware():
+    # GH 54239, 54287
+    pa = pytest.importorskip("pyarrow")


we may need to set a minimum version here

Suggested change

pa = pytest.importorskip("pyarrow")

pa = pytest.importorskip("pyarrow", "11.0.0")

thank you, I added the minimum version here

MarcoGorelli

minor comments, but the rest looks good to me!

MarcoGorelli · 2023-07-31T17:43:11Z

doc/source/whatsnew/v2.1.0.rst

@@ -682,6 +682,7 @@ Other
 - Bug in :class:`DataFrame` and :class:`Series` raising for data of complex dtype when ``NaN`` values are present (:issue:`53627`)
 - Bug in :class:`DatetimeIndex` where ``repr`` of index passed with time does not print time is midnight and non-day based freq(:issue:`53470`)
 - Bug in :class:`FloatingArray.__contains__` with ``NaN`` item incorrectly returning ``False`` when ``NaN`` values are present (:issue:`52840`)
+- Bug in :func:`api.interchange.from_dataframe` was raising during interchanging from non-pandas tz-aware data containing ``NaN`` values (:issue:`54287`)


can we write

non-pandas tz-aware data containing null values

?
technically nan is different, and specifically refers to invalid mathematical operations like 0/0

oops, my mistake. I corrected that.

MarcoGorelli · 2023-07-31T17:45:36Z

pandas/core/interchange/from_dataframe.py

+            # SettingWithCopyError happens if the `data` appears
+            # to have 'NaT'. If this happens, copy the `data`.


"appears to" sounds a bit mysterious :) how about

`SettingWithCopyError` may happen for datetime-like with missing values

thank you, I changed the wording as you suggested.

MarcoGorelli

nice work! looks good to me on green

mroeschke · 2023-07-31T19:52:04Z

Thanks @natmokval

TST: add a test for interchanging from non-pandas tz-aware

c89377c

MarcoGorelli reviewed Jul 28, 2023

View reviewed changes

TST: correct the test for interchanging from non-pandas tz-aware

ddcf978

MarcoGorelli reviewed Jul 28, 2023

View reviewed changes

mroeschke reviewed Jul 28, 2023

View reviewed changes

natmokval added 2 commits July 28, 2023 20:08

check an error in the test

42aeee0

correct test

9448242

MarcoGorelli reviewed Jul 31, 2023

View reviewed changes

natmokval added 2 commits July 31, 2023 10:59

add extra except in set_nulls

da6fe09

fix bug during interchanging from non-pandas tz-aware

15a1fa6

natmokval changed the title ~~TST: add a test for interchanging from non-pandas tz-aware~~ BUG: during interchanging from non-pandas tz-aware data Jul 31, 2023

MarcoGorelli reviewed Jul 31, 2023

View reviewed changes

correct test

dd2c3eb

MarcoGorelli approved these changes Jul 31, 2023

View reviewed changes

natmokval added 2 commits July 31, 2023 19:58

correct wording

c575157

resolve conflict

0d5774e

natmokval marked this pull request as ready for review July 31, 2023 18:45

MarcoGorelli added this to the 2.1 milestone Jul 31, 2023

MarcoGorelli added the Interchange Dataframe Interchange Protocol label Jul 31, 2023

MarcoGorelli approved these changes Jul 31, 2023

View reviewed changes

mroeschke approved these changes Jul 31, 2023

View reviewed changes

mroeschke merged commit 329fe8f into pandas-dev:main Jul 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: during interchanging from non-pandas tz-aware data #54287

BUG: during interchanging from non-pandas tz-aware data #54287

natmokval commented Jul 28, 2023 •

edited

Loading

MarcoGorelli Jul 28, 2023

natmokval Jul 28, 2023

MarcoGorelli Jul 28, 2023

natmokval Jul 28, 2023 •

edited

Loading

MarcoGorelli Jul 28, 2023

natmokval Jul 28, 2023

natmokval Jul 28, 2023

mroeschke Jul 28, 2023

natmokval Jul 28, 2023

mroeschke Jul 28, 2023

natmokval Jul 28, 2023

MarcoGorelli Jul 31, 2023

natmokval Jul 31, 2023

MarcoGorelli Jul 31, 2023

natmokval Jul 31, 2023

MarcoGorelli left a comment

MarcoGorelli Jul 31, 2023

natmokval Jul 31, 2023

MarcoGorelli Jul 31, 2023

natmokval Jul 31, 2023

MarcoGorelli left a comment

mroeschke commented Jul 31, 2023

	result = from_dataframe(pa.table({"arr": arr}).to_pandas())
	result = from_dataframe(pa.table({"arr": arr}))

	pa = pytest.importorskip("pyarrow")
	pa = pytest.importorskip("pyarrow", "11.0.0")

		# SettingWithCopyError happens if the `data` appears
		# to have 'NaT'. If this happens, copy the `data`.

BUG: during interchanging from non-pandas tz-aware data #54287

BUG: during interchanging from non-pandas tz-aware data #54287

Conversation

natmokval commented Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natmokval Jul 28, 2023 • edited Loading

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

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

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

MarcoGorelli left a comment

Choose a reason for hiding this comment

mroeschke commented Jul 31, 2023

natmokval commented Jul 28, 2023 •

edited

Loading

natmokval Jul 28, 2023 •

edited

Loading