cpcloud / ipython-autotime

Time everything in IPython
Apache License 2.0
118 stars 12 forks source link

Feature request: also print timestamp #16

Closed sam-s closed 3 years ago

sam-s commented 4 years ago

Similar to the extension Execute Time, I would like to add the timestamp - when the command was run. I use EIN -- Emacs IPython Notebook which does not support the browser extension.

ddelange commented 3 years ago

Hi @sam-s,

Do you mean something like this, where the datetime sting would be the local system time like datetime.datetime.now().isoformat()?

- time: 295 µs
+ time: 295 µs (started: 2020-11-20T12:37:04.659710)

What do you think about this idea in general @cpcloud? Formatting wise there are also multiple possibilities of course.

Also would take into consideration that the operation of getting this datetime string is a good 3-4ms overhead on my system (doubling the amount of time stop() takes).

sam-s commented 3 years ago

Hi @ddelange , Yes, this is exactly what I want, except that instead of us precision I would prefer a TZ name. I don't think that overhead is all that important because this feature is mostly interesting for those users whose cells take a lot of time to compute. Thank you!

ddelange commented 3 years ago

I would agree a more human readable format would be fitting here (indeed microseconds seem overkill).

A debatable tradeoff is that timezone names like EST can have vastly different meanings, whereas the numerical form is deterministic and standardised.

This works on both py27 and py3 (in any case datetime.datetime.now() is timezone naive and wouldn't be an option):

>>> from time import strftime, localtime
>>> strftime('%Y-%m-%d %H:%M:%S %Z', localtime())
'2020-11-20 22:20:45 CET'
>>> strftime('%Y-%m-%d %H:%M:%S%z', localtime())
'2020-11-20 22:35:30+0100'
ddelange commented 3 years ago

I chose the numerical format (also see the deprecation footnote in python docs for the CET notation). The resulting string can't be parsed by datetime.datetime.fromisoformat due to a missing colon in the timezone (more on this here), but e.g. pendulum.parse picks it up fine.

sam-s commented 3 years ago

I think requiring an external package for parsing is not such a good idea, especially since adding a colon to make the built-in parser work is relatively easy. Thank you!

ddelange commented 3 years ago

You're right, more correct to add the colon. New commit added in the PR :)

- time: 5.29 ms (started: 2020-12-15 15:26:06+0100)
+ time: 5.29 ms (started: 2020-12-15 15:26:06 +01:00)