Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: linearly spaced date_range (GH 20808) #20846

Merged
merged 12 commits into from
May 3, 2018

Conversation

onnoeberhard
Copy link
Contributor

exp = np.array(['2018-04-24T00:00:00', '2018-04-25T12:00:00',
'2018-04-27T00:00:00'], dtype='datetime64[ns]')

assert (rng == exp).all()
Copy link
Member

Choose a reason for hiding this comment

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

You can use tm.assert_numpy_array_equal here

@@ -162,6 +162,21 @@ def test_date_range_ambiguous_arguments(self):
with tm.assert_raises_regex(ValueError, msg):
date_range(start, end, periods=10, freq='s')

def test_date_range_convenience_periods(self):
Copy link
Member

Choose a reason for hiding this comment

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

Could you also test this with the tz arg specified? Would also be good to test a tz where there is a day light savings transition between start and end.

@@ -450,6 +450,7 @@ Other Enhancements
- Updated ``to_gbq`` and ``read_gbq`` signature and documentation to reflect changes from
the Pandas-GBQ library version 0.4.0. Adds intersphinx mapping to Pandas-GBQ
library. (:issue:`20564`)
- :func:`pandas.core.indexes.datetimes.date_range` now returns a linearly spaced DatetimeIndex if ``start``, ``stop``, and ``periods`` are specified, but ``freq`` is not. (:issue:`20808`)
Copy link
Member

Choose a reason for hiding this comment

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

  • func:`date_range` should suffice here
  • DatetimeIndex --> ``DatetimeIndex``


# See https://github.com/pandas-dev/pandas/issues/20808
if freq is None and periods is not None and start is not None \
and end is not None:
Copy link
Member

Choose a reason for hiding this comment

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

A little bit cleaner to use if freq is None and com._all_not_none(periods, start, end):

raise TypeError(msg.format(periods=periods))

start = Timestamp(start)
end = Timestamp(end)
Copy link
Member

Choose a reason for hiding this comment

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

start and end should be normalized if normalize=True is passed to date_range

@jschendel
Copy link
Member

For consistency, it seems like this behavior should also be implemented for bdate_range, timedelta_range, and interval_range. Doesn't really make sense for period_range, since a freq is always required there, and can probably skip cdate_range since it's been deprecated (although not removed).

@codecov
Copy link

codecov bot commented Apr 27, 2018

Codecov Report

Merging #20846 into master will increase coverage by <.01%.
The diff coverage is 95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #20846      +/-   ##
==========================================
+ Coverage   91.81%   91.81%   +<.01%     
==========================================
  Files         153      153              
  Lines       49478    49484       +6     
==========================================
+ Hits        45429    45435       +6     
  Misses       4049     4049
Flag Coverage Δ
#multiple 90.21% <95%> (ø) ⬆️
#single 41.85% <70%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/indexes/datetimes.py 95.76% <95%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b02c69a...4d6cd23. Read the comment docs.

@pep8speaks
Copy link

pep8speaks commented Apr 27, 2018

Hello @onnoeberhard! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on May 02, 2018 at 19:48 Hours UTC

@onnoeberhard
Copy link
Contributor Author

Alright, thank you for all the feedback. I have implemented almost everything mentioned, except for the implementation of a similar feature in the functions bdate_range, timedelta_range and interval_range, because I have not yet used these functions and don't know how to properly implement it in an intuitive way.

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

also would be nice to add this to TDI

"""

# See https://github.com/pandas-dev/pandas/issues/20808
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to go in the DTI constructor itself. we already do some of this validation, it needs to fit in there. Further you don't need to worry about lots of other things that you are repeating, e.g. tz, which are already handled.

@jreback jreback added Enhancement Datetime Datetime data dtype labels Apr 30, 2018
@onnoeberhard
Copy link
Contributor Author

I have moved everything to the DTI constructor. With that I have removed the functionality to declare kwargs to to_datetime, but I think it is much cleaner overall like this.

@onnoeberhard
Copy link
Contributor Author

Why do the tests time out? Is my code responsible for that?

@jreback jreback added this to the 0.23.0 milestone May 3, 2018
@jreback jreback merged commit 28dbae9 into pandas-dev:master May 3, 2018
@jreback
Copy link
Contributor

jreback commented May 3, 2018

thank @onnoeberhard

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Datetime Datetime data dtype Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: date_range not working as it intuitively should when specifying start, end, and periods
5 participants