API/COMPAT: add pydatetime-style positional args to Timestamp constructor #12482

thejohnfreeman · 2016-02-27T15:47:56Z

closes API/COMPAT: add kw args to Timestamp constructor #10758
tests added / passed
passes git diff upstream/master | flake8 --diff
whatsnew entry

If more tests are desired, please let me know where to put them. Same for the whatsnew entry. This is my first pull request for Pandas.

cc @jreback

jreback · 2016-02-27T15:50:55Z

pandas/tslib.pyx

        cdef _TSObject ts
        cdef _Timestamp ts_base

+        if offset is not None and type(offset) is int:


use is_integer_object(offset)

jreback · 2016-02-27T15:54:47Z

pls add a whatsnew entry.

any luck with accepting the kwargs as well? (or too complicated)

thejohnfreeman · 2016-02-27T16:23:19Z

Should the whatsnew go in for 18.1 or something else?

kwargs got too complicated. I wanted to reach a working version of positional parameters first before building kwargs on top of it. I will try out an idea this weekend.

jreback · 2016-02-27T16:26:31Z

you can put in 0.18.1; if you finish up before we release we can just move

jreback · 2016-03-03T14:11:09Z

doc/source/whatsnew/v0.18.1.txt

@@ -22,6 +22,14 @@ New features
 Enhancements
 ~~~~~~~~~~~~

+- Timestamps accept positional parameters like `datetime.datetime`:
+


put Timestamp in double back-ticks

say: can now accept

put a link in for datetime.datetime back to the constructor (on the python documentation page)

@jorisvandenbossche do we have a nicer way of doing this (as opposed to a straight link), IIRC, sphinx allows us to use an intra-module link (like we do with numpy)

yes, you can just do :func:datetime.datetime`` and that will link to the standard library's method

jreback · 2016-05-07T17:47:18Z

@thejohnfreeman can you rebase / update & move whatsnew to 0.18.2

thejohnfreeman · 2016-05-09T21:42:24Z

Ok, I rebased. I have time this week to finish this patch. I wrote a longer comment asking a question, but I'll just go ahead and try the signature you gave above.

def __new__(cls,
        object ts_input, object offset=None, tz=None, unit=None,
        year=None, month=None, day=None,
        hour=None, minute=None, second=None, tzinfo=None):

jreback · 2016-05-09T22:08:49Z

pandas/tslib.pyx

+        # The parameter list folds together legacy parameter names (the first
+        # four) and positional parameter names from pydatetime (starting with
+        # `minute`).
+        #


good, this is a nice way to do this.

codecov-io · 2016-05-09T22:46:04Z

Current coverage is 84.12%

Merging #12482 into master will not change coverage

@@             master     #12482   diff @@
==========================================
  Files           138        138          
  Lines         50384      50384          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits          42382      42382          
  Misses         8002       8002          
  Partials          0          0

Powered by Codecov. Last updated by 82f54bd...79d63d4

jorisvandenbossche · 2016-05-10T13:56:36Z

@thejohnfreeman You will also need to update the Timestamp docstring

jreback · 2016-05-10T14:17:45Z

also I believe its trivial to close #11630 (or it might already be done). Add a test and should be straightforward.

thejohnfreeman · 2016-05-11T03:36:11Z

Take a look at my attempt at supporting both positional and keyword arguments. I removed some tests for TypeError; it just means we are more permissive. I'll fix up the doc string and add the other requested tests tomorrow.

jreback · 2016-05-11T12:32:40Z

pandas/tslib.pyx

+            return Timestamp(datetime(year, month, day, hour or 0,
+                minute or 0, second or 0, microsecond or 0, tzinfo),
+                tz=tzinfo)
+        if is_integer_object(offset):


jreback · 2016-05-11T12:33:05Z

whoosh @thejohnfreeman nice constuctor now! yeah prob need some more tests.

shoyer · 2016-05-11T20:47:55Z

Effectively, we are doing type dependent multiple dispatch here with the Timestamp constructor. To be honest, I'm not a big fan of adding multiple paths like this that do such different things with the arguments.

jreback · 2016-05-11T21:21:52Z

@shoyer that doesn't make any sense. Timestamp is a sub-class of datetime. It doesn't take the correct (positional, nor keyword args ATM). I don't see the problem here. Yes it also does a lot more, but that's exactly the point, it IS user friendly w/o be ambiguous.

thejohnfreeman · 2016-05-11T23:56:27Z

I updated the docstring and added tests. I'm considering this a first draft submitted for approval. If it's approved, I'll squash the commits and we can wait for Travis.

shoyer · 2016-05-12T16:42:39Z

What about deprecating passing a single object (string or datetime) to pd.Timestamp in favor of using pd.to_datetime? That way we could get back to only one constructor eventually.

It would not be the end of the world to add this alternate constructor but IMO it would not be a positive improvement to have both.

If to do do it, let's clearly document the two constructors at the top of the docstring for the class, e.g.,

Timestamp(ts_input, object_offset=None, tz=None, unit=None)
Timestamp(year, month, day[, hour[, minute[, second[, microsecond[, tzinfo]]]]])

Timestamp is...

I think sphinx can process that correctly but we should double check.

jreback · 2016-05-12T17:09:07Z

@shoyer why would you blow up a very useful interface?

Timestamp('20130101') is such a useful feature and you want to DEPRECATE?

the entire point is to REDUCE mental effort here. The whole notion of a user having to choose alternate constructors (and I know we had this discussion w.r.t. RangeIndex) is completely bonkers to me.

Sure if things are ambiguous, then you have to choose. In this case it is wholly unambiguous, replicates the existing (python datetime.datetime API), and provides very useful parsing abilities, not to mention its very light weight.

shoyer · 2016-05-12T17:25:52Z

Timestamp('20130101') is such a useful feature and you want to DEPRECATE?

I agree -- my proposal is not entirely serious. I do think we need to make hard choices like this to achieve reliable APIs.

the entire point is to REDUCE mental effort here. The whole notion of a user having to choose alternate constructors (and I know we had this discussion w.r.t. RangeIndex) is completely bonkers to me.

Just to prove I'm not crazy, here are some high lights from the first page of a Google search for "Python multiple constructors":
http://stackoverflow.com/questions/2164258/multiple-constructors-in-python
http://stackoverflow.com/questions/682504/what-is-a-clean-pythonic-way-to-have-multiple-constructors-in-python
http://www.erol.si/2016/01/python-and-multiple-constructors/
https://pythonconquerstheuniverse.wordpress.com/2010/03/17/multiple-constructors-in-a-python-class/ (read the comments)

I could go on, but you get the point.

You'll notice that the universal advice seems to be to skip the magic and use classmethods for alternate constructors instead of overloading __init__.

kawochen · 2016-05-12T20:11:46Z

If we are to add more code paths in the constructor, is it possible to make the entry points more obvious by calling specific from_* methods inside __new__ or __init__? After reading the doc string I feel like it might become harder for a casual contributor like me to debug the code. But I think this can be remedied by making the intention of each code path more visible in the code.

thejohnfreeman · 2016-05-12T21:46:27Z

Hi @kawochen, I too want to make the code paths understandable. I had added some comments to each new path to document its intention. Did they help clarify at all?

thejohnfreeman · 2016-05-13T00:54:14Z

I keep getting lint errors in the automated check. It doesn't say what the error is, and running trace8 locally doesn't give me any indication either.

The command "ci/script.sh" exited with 1.
1.00s$ ci/lint.sh
inside ci/lint.sh
discarding /Users/travis/miniconda/envs/pandas/bin from PATH
prepending /Users/travis/miniconda/envs/pandas/bin to PATH
NOT Linting

Any ideas what I need to do to get Travis to pass?

jreback · 2016-05-13T01:07:38Z

it's not lint errors but a failed test

https://travis-ci.org/pydata/pandas/jobs/129782361

thejohnfreeman · 2016-05-13T01:24:25Z

Ah, it expects Timestamp(None) to return Not a Time (NaT).

jreback · 2016-05-13T13:06:38Z

pandas/tslib.pyx

+    year : int
+    month : int
+    day : int
+    hour : int [optional]


I think these need to be: int, optional (of this form)

…ctor

jreback · 2016-05-13T23:16:38Z

lgtm. @thejohnfreeman nice explanations in the comments. I think it is clear.

@jorisvandenbossche @shoyer

comments?

jreback · 2016-05-13T23:17:27Z

@thejohnfreeman did you add the test for #11630 (e.g the repr expr from above)

thejohnfreeman · 2016-05-13T23:45:40Z

pandas/tseries/tests/test_tslib.py

+        # GH 11630
+        self.assertEqual(
+            repr(Timestamp(2015, 11, 12)),
+            repr(Timestamp('20151112')))


Here is the test for #11630, Github didn't do the best job linking it to your earlier comment.

jreback · 2016-05-20T14:16:52Z

thanks @thejohnfreeman great PR!

thejohnfreeman · 2016-05-20T16:14:51Z

Thank you!

jreback reviewed Feb 27, 2016
View reviewed changes

jreback added Datetime Datetime data dtype API Design labels Feb 27, 2016

thejohnfreeman force-pushed the GH-10758 branch from 8023662 to 31b61e9 Compare February 28, 2016 16:22

jreback reviewed Mar 3, 2016
View reviewed changes

thejohnfreeman force-pushed the GH-10758 branch 2 times, most recently from 18afb8e to b745b7d Compare March 5, 2016 15:26

thejohnfreeman force-pushed the GH-10758 branch from b745b7d to 1feb3e2 Compare May 9, 2016 21:29

jreback reviewed May 9, 2016
View reviewed changes

jreback mentioned this pull request May 10, 2016

Validate Timestamp with invalid offset raises ValueError #13116

Closed

4 tasks

jreback reviewed May 11, 2016
View reviewed changes

shoyer mentioned this pull request May 13, 2016

tf.constant should support pandas Series and DataFrame as input tensorflow/tensorflow#2318

Closed

jreback reviewed May 13, 2016
View reviewed changes

thejohnfreeman added 7 commits May 13, 2016 09:46

API/COMPAT: add pydatetime-style positional args to Timestamp constru…

0d6884b

…ctor

Support positional and keyword arguments for Timestamp

5c34c04

Fix docstring; add tests for kwargs

6fad30b

review fixes: versionadded, issue links, repr tests, legacy

9c1e2dc

PEP8

a334eab

Special object for missing first argument

0ac786f

Format comments, test for NaT

79d63d4

thejohnfreeman force-pushed the GH-10758 branch from 63f841e to 79d63d4 Compare May 13, 2016 14:49

jreback added this to the 0.18.2 milestone May 13, 2016

thejohnfreeman reviewed May 13, 2016
View reviewed changes

jreback closed this in 72164a8 May 20, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API/COMPAT: add pydatetime-style positional args to Timestamp constructor #12482

API/COMPAT: add pydatetime-style positional args to Timestamp constructor #12482

thejohnfreeman commented Feb 27, 2016

jreback Feb 27, 2016

jreback commented Feb 27, 2016

thejohnfreeman commented Feb 27, 2016

jreback commented Feb 27, 2016

jreback Mar 3, 2016

jorisvandenbossche Mar 3, 2016

jreback commented May 7, 2016

thejohnfreeman commented May 9, 2016

jreback May 9, 2016

codecov-io commented May 9, 2016 •

edited

Loading

jorisvandenbossche commented May 10, 2016

jreback commented May 10, 2016

thejohnfreeman commented May 11, 2016

jreback May 11, 2016

jreback commented May 11, 2016

shoyer commented May 11, 2016

jreback commented May 11, 2016

thejohnfreeman commented May 11, 2016

shoyer commented May 12, 2016

jreback commented May 12, 2016

shoyer commented May 12, 2016

kawochen commented May 12, 2016

thejohnfreeman commented May 12, 2016

thejohnfreeman commented May 13, 2016

jreback commented May 13, 2016

thejohnfreeman commented May 13, 2016

jreback May 13, 2016

jreback commented May 13, 2016

jreback commented May 13, 2016

thejohnfreeman May 13, 2016

jreback commented May 20, 2016

thejohnfreeman commented May 20, 2016

API/COMPAT: add pydatetime-style positional args to Timestamp constructor #12482

API/COMPAT: add pydatetime-style positional args to Timestamp constructor #12482

Conversation

thejohnfreeman commented Feb 27, 2016

jreback Feb 27, 2016

Choose a reason for hiding this comment

jreback commented Feb 27, 2016

thejohnfreeman commented Feb 27, 2016

jreback commented Feb 27, 2016

jreback Mar 3, 2016

Choose a reason for hiding this comment

jorisvandenbossche Mar 3, 2016

Choose a reason for hiding this comment

jreback commented May 7, 2016

thejohnfreeman commented May 9, 2016

jreback May 9, 2016

Choose a reason for hiding this comment

codecov-io commented May 9, 2016 • edited Loading

Current coverage is 84.12%

jorisvandenbossche commented May 10, 2016

jreback commented May 10, 2016

thejohnfreeman commented May 11, 2016

jreback May 11, 2016

Choose a reason for hiding this comment

jreback commented May 11, 2016

shoyer commented May 11, 2016

jreback commented May 11, 2016

thejohnfreeman commented May 11, 2016

shoyer commented May 12, 2016

jreback commented May 12, 2016

shoyer commented May 12, 2016

kawochen commented May 12, 2016

thejohnfreeman commented May 12, 2016

thejohnfreeman commented May 13, 2016

jreback commented May 13, 2016

thejohnfreeman commented May 13, 2016

jreback May 13, 2016

Choose a reason for hiding this comment

jreback commented May 13, 2016

jreback commented May 13, 2016

thejohnfreeman May 13, 2016

Choose a reason for hiding this comment

jreback commented May 20, 2016

thejohnfreeman commented May 20, 2016

codecov-io commented May 9, 2016 •

edited

Loading