ENH: (GH3863) Timestamp.min and Timestamp.max return a valid Timestamp
State: closed

closes https://github.com/pydata/pandas/issues/3863 by adding valid min and max fields to the Timestamp class.


Labels:

Comments:
would you add a mention in release notes under API?

also I think you might mention min/max here: http://pandas.pydata.org/pandas-docs/dev/gotchas.html#minimum-and-maximum-timestamps

Could you also add a testcase :)

maybe instead of the actual values, should define them as `np.iinfo(np.int64).max/min` ?

with min + 1 (to avoid NaT conflict)

@jreback: Good idea about using np.iinfo - as a matter of fact, that's what I did initially but then I found that the constants were already defined but commented out. I'll add the change to the release notes and gotchas as well.

@hayd: With regards to tests - I'm not entirely sure what to test there? It is basically 2 fields. I can add some tests that assert the fields value...

Interestingly enough, on osx `np.iinfo(np.int64).min` returns a number that causes Timestamp to wrap around and return a value near the upper boundary:

```
In [1]: pd.Timestamp(np.iinfo(np.int64).max)
Out[1]: Timestamp('2262-04-11 23:47:16.854775807', tz=None)

In [2]: pd.Timestamp(np.iinfo(np.int64).min)
Out[2]: NaT

In [3]: pd.Timestamp(np.iinfo(np.int64).min + 1)
Out[3]: Timestamp('2262-04-10 00:12:43.145224193', tz=None)
```

This could be the reason why the magic number is used - and also mentioned in the gotchas.

Are you just asserting that these don't raise? (i.e. call them in a test is enough)

```
pd.Timestamp(pd.Timestamp.min)
pd.Timestamp(pd.Timestamp.max)
```

Actually check they equals whatever they're supposed to equal?

@SleepingPills can you hook up travis, then recommit this?

@SleepingPills maybe just define both of these as `np.iinfo(np.int64).max` and `min` (and don't add 1!),
rather than hardcode

@SleepingPills can you define these as `np.info(np.int64).max` and `min`?

moving to 0.12

Sorry for the delay, I've been waylaid by some urgent stuff.

I definitely wouldn't use np.iinfo(np.int64).min + 1 because of the wraparound. I'm not sure I understand the reasoning behind defining Timestamp.min as np.iinfo(np.int64).min. Wouldn't this simply mean that Timestamp.min == NaT?

I mean defining the min num as `np.iinfo(np.int64).min+1`; wraparound shouldn't affect this

also don't put the tests in a separate file; put in `tseries/tests/test_timeseries.py`

Even counting the fact that many other things, e.g. Timestamp.to_pydatetime() will behave really badly as a result?

what do you mean `Timestamp.to_pydatetime()` will behave bady? can you give an example?

Well:

```
In [1]: ts = pd.Timestamp(np.iinfo(np.int64).min+1)

In [2]: ts
Out[3]: Timestamp('2262-04-10 00:12:43.145224193', tz=None)

In [3]: ts == "20120101"
Out[3]: False

In [4]: ts == ts.to_pydatetime()
Warning: discarding nonzero nanoseconds
Out[4]: False

In [5]: ts.to_pydatetime()
Warning: discarding nonzero nanoseconds
Out[5]: datetime.datetime(2262, 4, 10, 0, 12, 43, 145224)
```

We now get a completely wrong datetime object. Not counting the fact that we lose precision by the conversion, the way it would behave with the np.iinfo() solution is worse (IMO).

sorry...yes you are right...the min datetime is a fixed value (and < np.iinfo(np.int64).min).....ok....fixed values are fine (I think wes avoided this whole wraparound issue on diff platforms by avoiding values that are too close to the min value)....

pls move the tests to tseries/test/time_series..and can merge this

Ok, I moved the tests and added an extra case for the datetime conversion roundtrip.

perfect, could you add a release notes mention? (doc/sources/release.rst)

and then squash down to a single commit?

thanks

Done. I thought I had pushed the changes to the docs already but turns out I had them on a different machine.

How do I squash already pushed commits?

```
git checkout master
git pull
git branch mybranch --set-upstream master
git checkout mybranch
git pull --rebase
```

fix any merge issues

when done rebasing

```
git rebase -i <commit just before this PR>
```

use `s` and/or `f`

when done

`git push myfork mybranch -f`

will replace all existing commits with the new set

generally on something like this is pretty short, can squash down to 1-2

related is #4065, if you would like to tackle would be appreciated (separate PR)

Squashed

thanks for the PR! merged

Thanks for the assistance - I'll try to make it smoother going forward.