ENH: support .strftime for datetimelikes (closes #10086) by mortada · Pull Request #10110 · pandas-dev/pandas (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation44 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

this works for DatetimeIndex, something broken for PeriodIndex. And should not work for TimedeltaIndex (as I don't think the strftime codes are meaningful). Further want to actually define .strftime on the index itself as well (maybe def strftime(self, format):).

needs tests for the same

that makes sense

I've added to both DatetimeIndex and PeriodIndex. Also added a few tests.

pls add strftime to API.rst
also I think a mini section in time series.rst would be ok too

Choose a reason for hiding this comment

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

hmm, not to make this more complicated, but maybe move to tseries/base/DatelikeOps (new mixin), that both DatetimeIndex/PeriodIndex, but NOT TimedeltaIndex inherit from.

Choose a reason for hiding this comment

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

I thought about something like that too, good idea for code reuse! Will update shortly

@jreback I added a mini section to basics.rst instead, but otherwise this is ready for review

Choose a reason for hiding this comment

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

maybe be explict and say that this is a string

can you update for comments. also related to #10442. I want this method to be the 'primary' date-string formatting (though .astype(str) will be supported by calling what this calls, namely .format), but this should be the goto method

copy your example that you added to basics.rst and put in 0.17.0 (in a separate section, maybe datetime-string formatting or somesuch)

@jreback updated according to your comments except for one thing: I couldn't actually find an invalid formatting string that would cause strftime to raise an exception

Choose a reason for hiding this comment

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

can u add a reference to the standard library docs for the strftime format here

Choose a reason for hiding this comment

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

I think this all needs to be indented 2 spaces (to form a sub-block)

We also have a tslib.format_array_from_datetime function. Can you check if this is faster? (also uses strftime, but the loop is cythonized)

Choose a reason for hiding this comment

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

put the if on the outside (and lambdas on the inside), then check is only done once

@jorisvandenbossche this calls .format(date_format=...) which ultimately calls the cython tslib.format_array_from_datetime, so this is just syntatic sugar.

@jreback reordered the lambdas and the if/else as you suggested, also added more unit tests

ok, lgtm. ping when green.

Choose a reason for hiding this comment

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

you can use :meth:Series.dt.strftime`` here as well, then it makes the link to the doc page

Choose a reason for hiding this comment

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

although that is maybe not that important here as it is exactly the same as standard library

Choose a reason for hiding this comment

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

sure will do

Choose a reason for hiding this comment

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

change back. we don't use np.testing.* directly

Choose a reason for hiding this comment

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

@jreback I changed it because assert_numpy_array_equal() actually would fail in some of the testing environments. It seems like a problem with the older numpy version (1.8.x)

what should I change it to instead?

Choose a reason for hiding this comment

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

ahh, you are not comparing numpy arrays (they are actually a list and an array), use exp = np.asarray(....) and I think it will work, you can also try tm.assert_almost_equal(...)

scratch that first part, use tm.assert_almost_equal; I though you were comparing the output of .strftime which was an np.array

Choose a reason for hiding this comment

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

ah cool I think tm.assert_almost_equal works, thanks! will update shortly

@mortada couple of comments. ping when green.

Labels