Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ArrayManager] Ensure to store datetimelike data as DatetimeArray/TimedeltaArray (and not ndarray) #40147

Conversation

jorisvandenbossche
Copy link
Member

Pre-cursor for #39991

Currently we didn't really check that we were consistently storing datetimelike data as the EA (DatetimeArray, TimedeltaArray) or as ndarrray. Ensuring this in the ArrayManager constructor turns up a few failures.

I think it will be the easiest to always store them as EA and not as ndarray (eg for many other operations, we otherwise would wrap them in the EA anyway).

@jorisvandenbossche jorisvandenbossche added Refactor Internal refactoring of code Internals Related to non-user accessible pandas implementation labels Mar 1, 2021
Comment on lines 740 to 743
elif is_datetime64_ns_dtype(dtype):
result = DatetimeArray._from_sequence(result, dtype=dtype)._data
elif is_timedelta64_ns_dtype(dtype):
result = TimedeltaArray._from_sequence(result, dtype=dtype)._data
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I am doing this here is because this fails:

In [4]: np.array([pd.NaT], dtype="M8[ns]")
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-4-99d42e913a1c> in <module>
----> 1 np.array([pd.NaT], dtype="M8[ns]")

ValueError: cannot convert float NaN to integer

(which is what the np.array([arr[loc] for arr in self.arrays], dtype=temp_dtype) above can be doing if the resulting dtype is M/m8)

@jbrockmendel do you know if that's something we do elsewhere as well / there is some existing code for this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC we can get BM to mess up if we get here with non-consolidated all-td64 blocks.

I think the thing to do (also similar change in the BM method) is to define result on L737 as a list, only wrap with np.array if none of these conditions hold

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC we can get BM to mess up if we get here with non-consolidated all-td64 blocks.

You can't get here in that case, though, since this is an ArrayManager method. And with the current BM you don't have this problem since it doesn't store DatetimeArray/TimedeltaArray with numpy dtypes.

I think the thing to do (also similar change in the BM method) is to define result on L737 as a list, only wrap with np.array if none of these conditions hold

Thanks, that's a good idea

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated with this simplification.

@jorisvandenbossche
Copy link
Member Author

@jbrockmendel more comments here?

# for datetime64/timedelta64, the np.ndarray constructor cannot handle pd.NaT
elif is_datetime64_ns_dtype(dtype):
result = DatetimeArray._from_sequence(values, dtype=dtype)._data
elif is_timedelta64_ns_dtype(dtype):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have a little-used is_ea_or_datetimelike_dtype, could use an analogous helper to get DatetimeArray/TimedeltaArray in these cases (not for this PR)

elif is_timedelta64_ns_dtype(dtype):
result = TimedeltaArray._from_sequence(values, dtype=dtype)._data
else:
result = np.array(values, dtype=dtype)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you check if this is relevant for the BlockManager case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, see my (somewhat) answer at #40147 (comment).
But moreover, in the BlockManager method, it assigns slices from the Block values into the resulting array:

for blk in self.blocks:
# Such assignment may incorrectly coerce NaT to None
# result[blk.mgr_locs] = blk._slice((slice(None), loc))
for i, rl in enumerate(blk.mgr_locs):
result[rl] = blk.iget((i, loc))

So that's quite different as the code here, and the idea of first keeping it in a list doesn't really apply.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

@jbrockmendel
Copy link
Member

LGTM cc @jreback

@jorisvandenbossche jorisvandenbossche merged commit 4f18821 into pandas-dev:master Mar 2, 2021
@jorisvandenbossche jorisvandenbossche deleted the am-datetimelike-storage branch March 2, 2021 21:08
@jorisvandenbossche
Copy link
Member Author

@jbrockmendel thanks for the review

I updated #39991 now based on this

@jorisvandenbossche jorisvandenbossche added this to the 1.3 milestone Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internals Related to non-user accessible pandas implementation Refactor Internal refactoring of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants