[ArrayManager] TST: arithmetic test #39753
Conversation
jorisvandenbossche
added
Testing
| # https://github.com/pandas-dev/pandas/issues/39750 | ||
| # third column with all-NaT as result doesn't get preserved | ||
| # as timedelta64 dtype | ||
| expected[2] = Series([pd.NaT, pd.NaT], dtype=object) |
There was a problem hiding this comment.
this is sub-optimal. can you add a TODO to try to retain the correct dtype
There was a problem hiding this comment.
I don't fully understand your comment. Of course it's sub-optimal, as it's a bug (but an existing one, and I opened an issue about it)
There was a problem hiding this comment.
but an existing one
now im confused; we only get here if using_array_manager
There was a problem hiding this comment.
i.e. if the expected isnt actually what we want, better to xfail
There was a problem hiding this comment.
now im confused; we only get here if using_array_manager
Yes, I "uncovered" the bug by adding array manager tests, but the bug itself is not related to ArrayManager, it's a bug in TimedeltaArray (and you can run into it without ArrayManager as well, just not covered by any test)
But indeed, an xfail might be more appropriate since I am now asserting the buggy behaviour. The problem is that I would then no longer test the other parts of this test (the other columns) which is actually working fine.
So I would prefer to keep it this way (but will update the comment to make it more clear this is buggy)
There was a problem hiding this comment.
ok can you open an issue to address this in BM. (and ideally an INFO / TODO) for future reference. I am getting increasinly worried that things are changed, but we have no idea where except for PR comments, which are not very useful forward looking.
There was a problem hiding this comment.
As mentioned above, and already included in the comment as well, I already opened an issue (it's #39750)
There was a problem hiding this comment.
and you can run into it without ArrayManager as well, just not covered by any test
OK. Can you add a non-ArrayManager test that will hit it?
There was a problem hiding this comment.
I added additional examples that don't involve ArrayManager to the issue (#39750)
|
one request #39753 (comment), otherwise LGTM |
| # https://github.com/pandas-dev/pandas/issues/39750 | ||
| # third column with all-NaT as result doesn't get preserved | ||
| # as timedelta64 dtype | ||
| expected[2] = Series([pd.NaT, pd.NaT], dtype=object) |
There was a problem hiding this comment.
ok can you open an issue to address this in BM. (and ideally an INFO / TODO) for future reference. I am getting increasinly worried that things are changed, but we have no idea where except for PR comments, which are not very useful forward looking.
| @@ -1761,6 +1763,8 @@ def test_td64arr_floordiv_td64arr_with_nat(self, box_with_array): | |||
|
|
|||
| expected = np.array([1.0, 1.0, np.nan], dtype=np.float64) | |||
| expected = tm.box_expected(expected, xbox) | |||
| if box is DataFrame and using_array_manager: | |||
| expected[[0, 1]] = expected[[0, 1]].astype("int64") | |||
There was a problem hiding this comment.
why is expected different and is one behavior more desirable than the other?
There was a problem hiding this comment.
ArrayManager performs the op column-wise, and thus can preserve dtypes (floordiv gives ints). Will add a comment about it in the line of https://github.com/pandas-dev/pandas/pull/39753/files#r576350947
xref #39146