BUG: preserve object-dtype index when accessing DataFrame column / PERF: improve perf of Series fastpath constructor

[jorisvandenbossche]

This came up while discussing the CoW proof of concept (and need for item_cache / speed of creating a Series of a column). Those small changes gives a decent reduction of some overhead (~30% reduction):

arr = np.array([1, 2, 3])
idx = pd.Index(["a", "b", "c"])

pd.Series(arr, index=idx, name="name", fastpath=True)

In [5]: %timeit pd.Series(arr, index=idx, name="name", fastpath=True)
10.9 µs ± 202 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)  # <-- master
7.48 µs ± 187 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)  # <-- PR

[jbrockmendel]

nice!

[jorisvandenbossche]

Apparently this is being relied upon in the pytables code though ..

And looking into it further, the change I made here of course changes behaviour, as it no longer try to infer object dtype as datetime in the fastpath.

[jorisvandenbossche]

Although that should probably be considered as a bug fix. With master:

In [1]: df = pd.DataFrame({'a': [1, 2, 3]}, index=pd.date_range("2012", periods=3).astype(object))

In [2]: df.index
Out[2]: Index([2012-01-01 00:00:00, 2012-01-02 00:00:00, 2012-01-03 00:00:00], dtype='object')

In [3]: df['a'].index
Out[3]: DatetimeIndex(['2012-01-01', '2012-01-02', '2012-01-03'], dtype='datetime64[ns]', freq=None)

where the index of the column-as-Series becomes different ..

[jbrockmendel]

IIRC we deprecated this, then un-deprecated bc we didn't have good warning handling/suppression, and were going to try to re-deprecate before long

[jorisvandenbossche]

Do you have a reference (issue/PR) for this?

[jorisvandenbossche]

There is #36697, but that's for the non-fastpath way, I think? While here it's specifically the fastpath that I am changing (which is not used when a user creates a Series directly themselves).

[jbrockmendel]

when we did that i dont remember any distinction being made between fastpath vs non-fastpath

[github-actions]

This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.

[mroeschke]

Appears this PR has been dormant for a while closing to clear the queue. Feel free to reopen when you have time to work on this PR and address the comment.

[jreback]

@jorisvandenbossche not averse to these PRs but can you merge master & address any comments esp on the older ones

[jbrockmendel]

Current thought: the if labels._is_all_dates is a PITA that we should rip out in 2.0 (we tried deprecating before and it was messy). Could we just avoid going through these paths by using _from_mgr in the relevant places?

[jorisvandenbossche]

Could we just avoid going through these paths by using _from_mgr in the relevant places?

I suppose you mean as alternative to passing fastpath=True, right?

First, the Series(..) fastpath still sets a name as well (which is not included in _from_mgr, so that would make replacing it more cumbersome, or a name keyword could be added to _from_mgr of course)

Looking at where fastpath=True is currently used, this is limited to:

• DataFrame._box_col_values (for constructing a Series for a DataFrame column) -> this uses _constructor_sliced, which needs to be called directly (we can't easily do _constructor_sliced._from_mgr for backwards compatibility, since _constructor is only required to be a callable, not necessarily a class)
• DataFrame._series attribute (only used in testing?)
• Series.take -> also uses _constructor (so similar issue as above)

So from that, I don't think it is super easy to change fastpath=True to _from_mgr

[jorisvandenbossche]

BTW, I still need to add a test case for this (the output is on master, with this PR both will be object dtype):

In [1]: df = pd.DataFrame({'a': [1, 2, 3]}, index=pd.date_range("2012", periods=3).astype(object))

In [2]: df.index
Out[2]: Index([2012-01-01 00:00:00, 2012-01-02 00:00:00, 2012-01-03 00:00:00], dtype='object')

In [3]: df['a'].index
Out[3]: DatetimeIndex(['2012-01-01', '2012-01-02', '2012-01-03'], dtype='datetime64[ns]', freq=None)

And we need to decide if we see that as a bug fix.

[jbrockmendel]

And we need to decide if we see that as a bug fix.

definitely looks like a bugfix, nice catch

[jreback]

can you rebase i think we merged some parts of this elsewhere

[jorisvandenbossche]

i think we merged some parts of this elsewhere

Any more specific pointer?

[jbrockmendel]

LGTM cc @jreback

[jbrockmendel]

@jorisvandenbossche can you rebase

[jreback]

pls also rebase

[jreback]

move to 1.5