Added test test_datetimeField_after_setitem for issue #6942 #28790

anirudnits · 2019-10-04T15:51:53Z

Added test test_datetimeField_after_setitem in tests/generic/test_frame.py.

closes BUG: setitem with at not inferring dtype correctly #6942
test added
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff

…rame.py

mroeschke · 2019-10-04T16:58:12Z

pandas/tests/generic/test_frame.py

@@ -287,3 +289,36 @@ def test_deepcopy_empty(self):
        empty_frame_copy = deepcopy(empty_frame)

        self._compare(empty_frame_copy, empty_frame)
+
+    def test_datetimeField_after_setitem(self):


Mention the use of at in the title: test_datetime_setitem_with_at

mroeschke · 2019-10-04T16:58:38Z

pandas/tests/generic/test_frame.py

+        df.at[start, "timenow"] = datetime.today()  # initial time.
+        time1 = df.at[start, "timenow"]
+
+        time.sleep(1)  # sleep time of 1 second in between assignments.


These sleep calls are not needed.

Some tests were failing with AssertionError, so added those.

Why were AssertionErrors being raised?

I tried the same code on my computer and got the following results:
time1: 2019-10-04 23:23:31.911241
time2: 2019-10-04 23:23:31.911534
time3: 2019-10-04 23:23:31.912060
that is a time difference of 10^(-3) between the time1 and time2.Now if both the statements are executed within a time frame of <= 10^(-6) then the two will match. So that's why I thought of adding the sleep in between time1, time2 and time3 to ascertain this doesn't happen.

The tests should generally be

result = pd.DataFrame(index=pd.date_range(start,periods=1), columns=['timenow','Live']) var = datetime.today() result.at[start,'timenow'] = var result.Live = True expected = pd.DataFrame([[var, True]],index=pd.date_range(start,periods=1), columns=['timenow','Live']) tm.assert_frame_equal(result, expected)

Sorry I misread the problem and was writing the test to check if the timenow column changes value after setting the Live column with "at".
So, what the test should check is that the dtype of timenow column remains unchanged after setting the Live column to True with at. For that I could use the assert_series_equal and check the dtype of timenow before and after.
Is that right?

This test should check that the timenow column changes once df.Live = True

i.e. the example in #6942 (comment) is the fixed behavior that we want to test for.

Some points:

For the general template of tests that is used in pandas/tests, most of them assert that either two DataFrames or Series are equal and call the predefined functions tm.assert_something_equal. However in this case we assert the column actually changes value and I couldn't find a function for such a case in tm and therefore wrote the individual assert statements for the different values in the timenow column(asserting that they are not equal).

Without the sleep statements inserted, some checks were failing. When I viewed the log of the failed tests I observed that the error was because
assert not time1 == time2
was evaluating to
assert not datetime.datetime(2019, 10, 4, 16, 12, 38, 392372) == datetime.datetime(2019, 10, 4, 16, 12, 38, 392372)
So I realized that this could only occur if both the statements are run within a time-frame < 10^(-6). When I added the sleep statements the checks passed.

You will still use tm.assert_frame_equal. You will have to construct the expected dataframe from scratch

In [4]: result = pd.DataFrame(index=pd.date_range(start,periods=1), columns=['timenow','Live']) In [5]: result.at[start,'timenow'] = datetime.today() # initial value In [7]: new_date = datetime.today() In [9]: result.Live = True In [10]: result.at[start,'timenow'] = new_date In [11]: expected = pd.DataFrame([[new_date, True]], index=pd.date_range(start,periods=1), columns=['timenow','Live']) In [12]: tm.assert_frame_equal(result, expected)

Thanks, couldn't come up with that! Will make the necessary changes and resubmit the PR.

mroeschke · 2019-10-04T16:58:49Z

pandas/tests/generic/test_frame.py

+
+        df.Live = True  # setting the 'Live' column to True.
+
+        time.sleep(1)  # sleep time of 1 second in between assignments.


These sleep calls are not needed.

Some checks were failing with AssertionError, so I added those

mroeschke · 2019-10-04T17:00:23Z

pandas/tests/generic/test_frame.py

+        ] = datetime.today()  # modified time after 'Live' column is set.
+        time3 = df.at[start, "timenow"]
+
+        assert not time1 == time2


We would like to construct 2 dataframes here and use assert_frame_equal to compare them

result = ... expected = ... tm.assert_frame_equal(result, expected)

I thought of the same but the whole point of the test is to make sure that the datetimeField changes with each assignment and as such would require to assert that frames are not equal, that's went with this. Couldn't find a more elegant way round this.

…test_frame.py

pep8speaks · 2019-10-08T19:16:27Z

Hello @anirudnits! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-01-04 04:19:00 UTC

mroeschke · 2019-10-08T20:47:16Z

pandas/tests/generic/test_frame.py

+            columns=["timenow", "Live"],
+        )
+
+        assert_frame_equal(result, expected, check_dtype=False)


check_dtype=True should be set here.

mroeschke · 2019-10-08T20:47:27Z

pandas/tests/generic/test_frame.py

+            index=pd.date_range(start, periods=1), columns=["timenow", "Live"]
+        )
+
+        result.at[start, "timenow"] = datetime.today()  # initial datetime.


you can remove this comment.

mroeschke · 2019-10-08T20:47:44Z

pandas/tests/generic/test_frame.py

@@ -287,3 +288,30 @@ def test_deepcopy_empty(self):
        empty_frame_copy = deepcopy(empty_frame)

        self._compare(empty_frame_copy, empty_frame)
+
+    def test_datetimeField_after_setitem_with_at(self):


Nit: test_datetime_field_after_setitem_with_at

WillAyd · 2019-11-07T23:10:38Z

@anirudnits is this still active? Can you merge master and repush?

anirudnits · 2019-11-08T06:38:31Z

@WillAyd sure will do that.

…d_test_in_test_frame.py_for_issue_#6942 Need to merge with the master branch to make a pull request to the main repository

WillAyd · 2019-12-09T18:11:44Z

@anirudnits looks like this is failing - can you get CI green?

anirudnits · 2019-12-16T10:49:49Z

@WillAyd sorry for the late reply, I am little caught up with some things and will work on the same as soon as possible. Thanks

…d_test_in_test_frame.py_for_issue_#6942 Just updating the branch to the latest version from GITHUB

WillAyd · 2020-02-02T01:25:40Z

@anirudnits is this still active?

anirudnits · 2020-02-02T03:39:01Z

I guess the reason that CI is failing is because that the dtypes of the DataFrames are not equal (I don't know if that is by design or an error). I can make the check_dytpe = False in the assert_frame_equal function and I believe that will pass the CI and sorry to keep this issue idle for so long. Appreciate your patience.

WillAyd · 2020-03-14T21:47:12Z

Yea I don't think the original issue is fixed then - are you interested in trying to debug further?

anirudnits · 2020-03-18T12:45:43Z

@WillAyd yeah sure. I will work on it.

anirudnits · 2020-03-18T15:23:06Z

@WillAyd I will also mention the issue in the original thread #6942.

jbrockmendel · 2020-03-19T22:05:22Z

pandas/tests/generic/test_frame.py

@@ -280,3 +281,30 @@ def test_deepcopy_empty(self):
        empty_frame_copy = deepcopy(empty_frame)

        self._compare(empty_frame_copy, empty_frame)
+
+    def test_datetime_after_setitem_with_at(self):


this should go in tests.indexing.test_scalar

is "at" the function being tested? if so, the name should be something like "test_at_with..."

jbrockmendel · 2020-03-19T22:05:32Z

pandas/tests/generic/test_frame.py

+        # This test covers the unexpected behaviour of datetimeField when using
+        # setitem on another column as reported in issue #6942
+
+        start = pd.to_datetime("20140401")


can you use pd.Timestamp here

jbrockmendel · 2020-03-19T22:06:03Z

pandas/tests/generic/test_frame.py

@@ -280,3 +281,30 @@ def test_deepcopy_empty(self):
        empty_frame_copy = deepcopy(empty_frame)

        self._compare(empty_frame_copy, empty_frame)
+
+    def test_datetime_after_setitem_with_at(self):
+        # This test covers the unexpected behaviour of datetimeField when using


datetimeField -> datetime64 column ?

jbrockmendel · 2020-03-19T22:06:38Z

pandas/tests/generic/test_frame.py

+
+        new_datetime = datetime.today()
+
+        result.Live = True


does the test rely on this being result.Live instead of result["Live"]? if not, pls use the latter

jbrockmendel · 2020-03-19T22:07:09Z

pandas/tests/generic/test_frame.py

+
+        result.Live = True
+
+        # Changing the value in "timenow" column after "Live" colunn is set to True.


colunn -> column

jbrockmendel · 2020-03-19T22:08:10Z

pandas/tests/generic/test_frame.py

+
+        result.at[start, "timenow"] = datetime.today()
+
+        new_datetime = datetime.today()


does it matter that this datetime.today() is called separately from the one two lines up? if not, pls use just one datetime object

jbrockmendel · 2020-03-19T22:09:59Z

pandas/tests/generic/test_frame.py

+            index=pd.date_range(start, periods=1), columns=["timenow", "Live"]
+        )
+
+        result.at[start, "timenow"] = datetime.today()


is the "at" call being tested this one or the one below? if not this one, can you set this with a better-tested indexer (or just directly in the constructor call above)

@jbrockmendel yeah that could be done. I did that just to replicate the situation in the original issue thread as it mentioned that the "at" worked before setting an entire column(i.e the "Live" column) but not afterwards.

anirudnits · 2020-03-20T16:38:51Z

I guess this behavior with at is causing the issue:

With loc

f1 = pd.DataFrame(columns=["A"])
f1.loc[1] = [dt.now()]
f1.dtypes
A    datetime64[ns]
dtype: object

With the constructor

f1 = pd.DataFrame([[dt.now()]], columns=["A"])
f1.dtypes
A    datetime64[ns]
dtype: object

The same thing with at:

f1 = pd.DataFrame(columns=["A"])
f1.at[1, "A"] = dt.now()
f1.dtypes
A    object
dtype: object

mroeschke · 2020-05-23T04:37:26Z

@anirudnits do you have time to merge in master and address the review comments?

anirudnits · 2020-05-23T12:59:51Z

@mroeschke but wouldn't the CI still fail?

mroeschke · 2020-05-25T17:42:20Z

Are you interested in solving the original issue still?

anirudnits · 2020-05-25T17:47:25Z

@mroeschke sure, I'll look over the code and get back to you by this weekend. That's okay?

anirudnits · 2020-05-28T17:48:02Z

I found another peculiar behavior with dtypes and believe it to be unintended
df = pd.DataFrame(columns=["a", "b"])
df.loc[0, "a"] = 1
df.dtypes:
a: **object**
b: object
dtype: object
but
df = pd.DataFrame(columns=["a"])
df.loc[0, "a"] = 1
df.dtypes:
a: **int64**
dtype: object

shouldn't in the first snippet the dtype of column "a" be int64 just like in the second snippet?

mroeschke · 2020-05-29T06:50:46Z

Correct. I think both should be int64

anirudnits · 2020-06-06T11:18:19Z

@mroeschke Sorry for the delay in responding I had been stuck with my school work. So, along with the dtype issues mentioned in this thread, some other dtype issues especially with .loc have been mentioned in issues #11617, #14205, #14337 and #14361, some of these issues are closed without proper solution of the said problem. To add to these issues I found something interesting:

df = pd.DataFrame(columns=['A'])
df.loc[0, "A"] = 7
df.dtypes
A int64
dtype: object

But

df = pd.DataFrame(columns=['A'])
df.loc[0] = 7
df.dtypes
A object
dtype: object

So, I believe it has something to do with how loc sets an entire row versus a single column in a row(even though the row may have only one column). Using at in the above scenario gives the expected dtype of int64 both times. I tried to find the section in

pandas/pandas/core/indexing.py

Line 900 in 6258397

class _LocIndexer(_LocationIndexer):

, where I could find that difference in the behavior of loc in handling the two different cases. But the deeper the went I still couldn't find the exact code what I was looking for. So that's what I am stuck with now and would be thankful if you could point to the file(s) where I would find the desired code sections.

jreback · 2020-08-07T23:00:51Z

is this still active? this was supposed to be a simple test, but has morphed; let's keep the scope down here.

anirudnits · 2020-08-08T04:24:53Z

@jreback I got stuck with the whole indexer thing and couldn't find any real solutions to the dtype problem :( I can get the CI to pass by adding check_dtype=False when I check the equality of frames.

WillAyd · 2020-09-10T19:02:12Z

@anirudnits can you merge master and fix conflicts?

mroeschke · 2020-09-12T18:04:23Z

@anirudnits thanks for the PR. Going to close as stale but please let us know if you would like to pick up this issue again.

anirudnits added 2 commits October 4, 2019 21:12

Added test test_datetimeField_after_setitem for issue #6942 in test_f…

187ad13

…rame.py

Added sleep time of 1 second in between assignments

80c52d6

mroeschke reviewed Oct 4, 2019

View reviewed changes

anirudnits added 2 commits October 8, 2019 23:48

Added test test_datetimeField_after_setitem_with_at in tests/generic/…

4e02832

…test_frame.py

Added test test_datetimeField_after_setitem_with_at in tests/generic/…

93f0385

…test_frame.py

mroeschke reviewed Oct 8, 2019

View reviewed changes

Made the required changes

259013a

Merge branch 'master' of https://github.com/pandas-dev/pandas into ad…

768d553

…d_test_in_test_frame.py_for_issue_#6942 Need to merge with the master branch to make a pull request to the main repository

anirudnits added 2 commits January 4, 2020 09:19

Merge branch 'master' of https://github.com/pandas-dev/pandas into ad…

a4db260

…d_test_in_test_frame.py_for_issue_#6942 Just updating the branch to the latest version from GITHUB

Fixed a typo which was causing a ValueError

9cbf34c

jbrockmendel added the Testing pandas testing functions or related to the test suite label Mar 19, 2020

jbrockmendel reviewed Mar 19, 2020

View reviewed changes

mroeschke closed this Sep 12, 2020


		df.Live = True # setting the 'Live' column to True.

		time.sleep(1) # sleep time of 1 second in between assignments.


		result.Live = True

		# Changing the value in "timenow" column after "Live" colunn is set to True.


		result.at[start, "timenow"] = datetime.today()

		new_datetime = datetime.today()

Added test test_datetimeField_after_setitem for issue #6942 #28790

Added test test_datetimeField_after_setitem for issue #6942 #28790

Conversation

anirudnits commented Oct 4, 2019

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

mroeschke Oct 6, 2019 • 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

pep8speaks commented Oct 8, 2019 • edited Loading

Comment last updated at 2020-01-04 04:19:00 UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd commented Nov 7, 2019

anirudnits commented Nov 8, 2019

WillAyd commented Dec 9, 2019

anirudnits commented Dec 16, 2019

WillAyd commented Feb 2, 2020

anirudnits commented Feb 2, 2020

WillAyd commented Mar 14, 2020

anirudnits commented Mar 18, 2020

anirudnits commented Mar 18, 2020

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

anirudnits commented Mar 20, 2020

mroeschke commented May 23, 2020

anirudnits commented May 23, 2020

mroeschke commented May 25, 2020

anirudnits commented May 25, 2020 • edited Loading

anirudnits commented May 28, 2020

mroeschke commented May 29, 2020

anirudnits commented Jun 6, 2020 • edited Loading

jreback commented Aug 7, 2020

anirudnits commented Aug 8, 2020

WillAyd commented Sep 10, 2020

mroeschke commented Sep 12, 2020

mroeschke Oct 6, 2019 •

edited

Loading

pep8speaks commented Oct 8, 2019 •

edited

Loading

anirudnits commented May 25, 2020 •

edited

Loading

anirudnits commented Jun 6, 2020 •

edited

Loading