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

DEPR: Deprecate cdate_range and merge into bdate_range #17691

Merged
merged 3 commits into from Oct 2, 2017

Conversation

@jschendel
Copy link
Member

commented Sep 27, 2017

  • closes #17596
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

A bit going on here, so see summary below.

Deprecated cdate_range:

  • Removed from api (was added to api after 0.20.3 release)
  • Added a FutureWarning to cdate_range indicating deprecation
    • Added a test for this too
  • Converted other tests using cdate_range to use bdate_range instead
    • Cleaned up the entire test script to make it more consistent with others (i.e. with context)
  • Removed a reference to cdate_range from the current whatsnew (previous commit I wrote)

Expanded functionality of bdate_range:

  • Added the additional cdate_range params (weekmask, holidays) for custom frequency ranges
  • Expanded that functionality beyond what was previously present in cdate_range
    • cdate_range only supported cday with the weekmask and holidays params, now all custom freqs are supported.
    • Did this by checking if the passed freq starts with 'C'.
    • Added a test that should catch if a new non-custom freq is added that starts with 'C'.
    • Didn't add this to cdate_range; left that as-is.
  • Added a UserWarning if non-default weekmask or holidays are passed without a custom freq.
    • Added a test for this too.

Edits to timeseries.rst:

  • Added a custom freq bdate_range example to the "Generating Ranges of Timestamps" section
  • Reordered some of the examples in the "Generating Ranges of Timestamps" section
    • Tried to order examples by simplicity
  • Otherwise, mostly small rewording changes and other such small fixes (ticks, capitalization, etc.).
@codecov

This comment has been minimized.

Copy link

commented Sep 27, 2017

Codecov Report

Merging #17691 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17691      +/-   ##
==========================================
- Coverage   91.23%   91.21%   -0.02%     
==========================================
  Files         163      163              
  Lines       49796    49806      +10     
==========================================
+ Hits        45430    45431       +1     
- Misses       4366     4375       +9
Flag Coverage Δ
#multiple 89.01% <100%> (ø) ⬆️
#single 40.33% <30.76%> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/tseries/offsets.py 97.14% <ø> (-0.01%) ⬇️
pandas/core/indexes/datetimes.py 95.58% <100%> (+0.04%) ⬆️
pandas/core/api.py 100% <100%> (ø) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.73% <0%> (-0.1%) ⬇️

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 408ecd2...4cbf70d. Read the comment docs.

@jreback
Copy link
Contributor

left a comment

looks pretty good


.. _timeseries.representation:

Time Stamps vs. Time Spans
Timestamps vs. Time Spans

This comment has been minimized.

Copy link
@jreback

jreback Sep 27, 2017

Contributor

underlines should match the length exactly

``DatetimeIndex``. The default unit is nanoseconds, since that is how ``Timestamp``
objects are stored internally. However, epochs are often stored in another ``unit``
which can be specified. These are computed from the starting point specified by the
:ref:`Origin Parameter <timeseries.origin>`.

This comment has been minimized.

Copy link
@jreback

jreback Sep 27, 2017

Contributor

use origin

@@ -402,15 +392,29 @@ using various combinations of parameters like ``start``, ``end``,
pd.bdate_range(start=start, periods=20)
The start and end dates are strictly inclusive. So it will not generate any

This comment has been minimized.

Copy link
@jreback

jreback Sep 27, 2017

Contributor

put a small ::warning box where you say cdate_range is deprecated, add a ref as well (then you can refer to it in whatsnew)

This comment has been minimized.

Copy link
@jreback

jreback Sep 27, 2017

Contributor

in the appropriate location in this file

@@ -163,6 +163,7 @@ Other Enhancements
- :func:`Categorical.rename_categories` now accepts a dict-like argument as `new_categories` and only updates the categories found in that dict. (:issue:`17336`)
- :func:`read_excel` raises ``ImportError`` with a better message if ``xlrd`` is not installed. (:issue:`17613`)
- :meth:`DataFrame.assign` will preserve the original order of ``**kwargs`` for Python 3.6+ users instead of sorting the column names

This comment has been minimized.

Copy link
@jreback

jreback Sep 27, 2017

Contributor

move to deprecation section, mention the deprecation, and include a ref

@@ -491,6 +492,7 @@ Deprecations
- ``pd.options.html.border`` has been deprecated in favor of ``pd.options.display.html.border`` (:issue:`15793`).
- :func:`SeriesGroupBy.nth` has deprecated ``True`` in favor of ``'all'`` for its kwarg ``dropna`` (:issue:`11038`).
- :func:`DataFrame.as_blocks` is deprecated, as this is exposing the internal implementation (:issue:`17302`)
- ``cdate_range`` has been deprecated in favor of :func:`bdate_range` (:issue:`17596`)

This comment has been minimized.

Copy link
@jreback

jreback Sep 27, 2017

Contributor

combine with the previous

msg = 'invalid custom frequency string: {freq}'.format(freq=freq)
raise ValueError(msg)
elif holidays or (weekmask != 'Mon Tue Wed Thu Fri'):
warnings.warn('a custom frequency string was not passed, ignoring '

This comment has been minimized.

Copy link
@jreback

jreback Sep 27, 2017

Contributor

raise here, this is invalid

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Sep 28, 2017

Member

I agree this can raise.

@@ -525,20 +524,18 @@ def test_freq_divides_end_in_nanos(self):


class TestCustomDateRange(object):

This comment has been minimized.

Copy link
@jreback

jreback Sep 27, 2017

Contributor

separately, most of these classes could actually be a hierarchy instead to share some code (or could parametrize things), but separate / later PR.

with tm.assert_raises_regex(ValueError, msg.format(freq=bad_freq)):
bdate_range(START, END, freq=bad_freq)

def test_depr_cdaterange(self):

This comment has been minimized.

Copy link
@jreback

jreback Sep 27, 2017

Contributor

deprecation

@jsexauer jsexauer referenced this pull request Sep 27, 2017

Open

DEPR: deprecations from prior versions #6581

0 of 89 tasks complete
@jorisvandenbossche
Copy link
Member

left a comment

Thanks a lot for this!
Added a few comments


.. ipython:: python
pd.Timestamp(1349720105, unit='s')

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Sep 28, 2017

Member

I personally don't find it necessary to add an example using Timestamp, IMO we should 'sell' to_datetime as the function to convert things to timestamps. (you can also convert a scalar with to_datetime if you want)

frequency, we can use the pandas functions ``date_range`` and ``bdate_range``
to create timestamp indexes.
frequency, we can use the ``date_range`` and ``bdate_range`` functions
to create a ``DatetimeIndex``. The default frequency for ``date_range`` is a

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Sep 28, 2017

Member

can you make date_range and bdate_range references to their docstring? (:func:`date_range` )

dates outside of those dates if specified.
.. versionadded:: 0.21.0

``bdate_range`` can also generate a range of custom frequency dates by using

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Sep 28, 2017

Member

I think if you indent this paragraph, it more 'belongs' to the versionadded directive (although not fully sure how it will look like when rendered)

This comment has been minimized.

Copy link
@jschendel

jschendel Sep 29, 2017

Author Member

I tried indenting and it rendered everything on the same line, i.e. "New in version 0.21.0: bdate_range can also...". I changed it back to make it consistent with the other usages of versionadded within timeseries.rst and other documentation.

@@ -2049,7 +2051,8 @@ def date_range(start=None, end=None, periods=None, freq='D', tz=None,


def bdate_range(start=None, end=None, periods=None, freq='B', tz=None,
normalize=True, name=None, closed=None, **kwargs):
normalize=True, name=None, weekmask='Mon Tue Wed Thu Fri',

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Sep 28, 2017

Member

I would put the weekmask default to None? (as it is only used when freq is something else, so this gives a bit a misleading signature?

And if freq='C' is passed and weekmask is None, you can set it to this value.
(or is None a valid value when freq='C' ?)

@@ -2071,6 +2074,19 @@ def bdate_range(start=None, end=None, periods=None, freq='B', tz=None,
Normalize start/end dates to midnight before generating date range
name : string, default None
Name of the resulting DatetimeIndex
weekmask : string, default 'Mon Tue Wed Thu Fri'
weekmask of valid business days, passed to ``numpy.busdaycalendar``,

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Sep 28, 2017

Member

weekmask -> Weekmask (also the description for 'holidays' can start with capital)

msg = 'invalid custom frequency string: {freq}'.format(freq=freq)
raise ValueError(msg)
elif holidays or (weekmask != 'Mon Tue Wed Thu Fri'):
warnings.warn('a custom frequency string was not passed, ignoring '

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Sep 28, 2017

Member

I agree this can raise.

@@ -2137,6 +2165,9 @@ def cdate_range(start=None, end=None, periods=None, freq='C', tz=None,
-------
rng : DatetimeIndex
"""
warnings.warn("cdate_range is deprecated and will be removed in a future "
"version, instead use bdate_range(..., freq='{freq}')"

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Sep 28, 2017

Member

bdate_range -> pd.bdate_range (to make it clear that bdate_range is top-level, as opposed to cdate_range)

@jorisvandenbossche jorisvandenbossche added this to the 0.21.0 milestone Sep 28, 2017

@jschendel jschendel force-pushed the jschendel:depr-cdate_range branch from eb38d12 to 7306a73 Sep 29, 2017

@jschendel

This comment has been minimized.

Copy link
Member Author

commented Sep 29, 2017

Made the review changes and fixed some unrelated typos I noticed in whatsnew (missing : and `).

@jorisvandenbossche
Copy link
Member

left a comment

Looks good!

can be represented using a 64-bit integer is limited to approximately 584 years:

.. ipython:: python
pd.Timestamp.min
pd.Timestamp.max
See :ref:`here <timeseries.oob>` for ways to represent data outside these bound.

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Sep 29, 2017

Member

I think here the "for ways to represent data outside these bound" is actually useful to know that it is interesting to click this link

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Sep 29, 2017

Member

Ah, but the 'see also' block will show the full title, which is "representing out of bounds spans", which also says it.

pd.bdate_range(start, end, freq='CBMS', weekmask=weekmask)
.. warning::

This comment has been minimized.

Copy link
@jreback

jreback Oct 1, 2017

Contributor

move this to the top of the subsection. That way you can in the whatsnew refer directly to this section.

@@ -657,9 +658,9 @@ Numeric

Categorical
^^^^^^^^^^^
- Bug in :func:`Series.isin` when called with a categorical (:issue`16639`)
- Bug in :func:`Series.isin` when called with a categorical (:issue:`16639`)

This comment has been minimized.

Copy link
@jreback

jreback Oct 1, 2017

Contributor

thanks for fixing this!

@@ -63,7 +63,7 @@ class TestPDApi(Base):
# top-level functions
funcs = ['bdate_range', 'concat', 'crosstab', 'cut',
'date_range', 'interval_range', 'eval',
'factorize', 'get_dummies', 'cdate_range',
'factorize', 'get_dummies',

This comment has been minimized.

Copy link
@jreback

jreback Oct 1, 2017

Contributor

move to the deprecated functions section, and add a test (in same module), model after the top-level deprecations.

@@ -3002,5 +3003,3 @@ def generate_range(start=None, end=None, periods=None,
FY5253,
FY5253Quarter,
])

This comment has been minimized.

Copy link
@jreback

jreback Oct 1, 2017

Contributor

why did you change this?

This comment has been minimized.

Copy link
@jschendel

jschendel Oct 1, 2017

Author Member

Cleaning. Seemed a bit strange that Nano was being added to the dict after creation. Looks like it was historically structured that way for compat with numpy < 1.7:

if not _np_version_under1p7:
# Only 1.7+ supports nanosecond resolution
prefix_mapping['N'] = Nano

Structure was kept the same when the if statement was removed, but seems fine to directly add it to the dict during creation now.

This comment has been minimized.

Copy link
@jreback

jreback Oct 1, 2017

Contributor

k cool

@jschendel jschendel force-pushed the jschendel:depr-cdate_range branch from 7306a73 to 4cbf70d Oct 2, 2017

@jschendel

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2017

Made the additional review changes.


def test_deprecation_cdaterange(self):
# GH17596
from pandas.core.indexes.datetimes import cdate_range

This comment has been minimized.

Copy link
@jreback

jreback Oct 2, 2017

Contributor

import from pandas here (we are actually testing the pd. deprecation); in this case they are the same, but its our practice to use the one we are deprecating.

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Oct 2, 2017

Member

no, we are testing this one (the move to top-level pd namespace was only very recently in master, so we don't have to deprecate that. That one we just removed, and we are deprecating the actual nested one)

This comment has been minimized.

Copy link
@jreback

jreback Oct 2, 2017

Contributor

oh, ok then; guess we moved this in 0.21.0, so this is fine then.

@jreback

jreback approved these changes Oct 2, 2017

Copy link
Contributor

left a comment

just a minor comment.

@jreback

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2017

lgtm. @jorisvandenbossche @TomAugspurger if you have anything further.

@jreback jreback merged commit 2781b18 into pandas-dev:master Oct 2, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jreback

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2017

thanks @jschendel

ghost pushed a commit to reef-technologies/pandas that referenced this pull request Oct 2, 2017

Krzysztof Chomski
Merge branch 'master' of github.com:pandas-dev/pandas
* 'master' of github.com:pandas-dev/pandas: (188 commits)
  Separate out _convert_datetime_to_tsobject (pandas-dev#17715)
  DOC: remove whatsnew note for xref pandas-dev#17131
  BUG: Regression in .loc accepting a boolean Index as an indexer (pandas-dev#17738)
  DEPR: Deprecate cdate_range and merge into bdate_range (pandas-dev#17691)
  CLN: replace %s syntax with .format in pandas.core: categorical, common, config, config_init (pandas-dev#17735)
  Fixed the memory usage explanation of categorical in gotchas from O(nm) to O(n+m) (pandas-dev#17736)
  TST: add backward compat for offset testing for pickles (pandas-dev#17733)
  remove unused time conversion funcs (pandas-dev#17711)
  DEPR: Deprecate convert parameter in take (pandas-dev#17352)
  BUG:Time Grouper bug fix when applied for list groupers (pandas-dev#17587)
  BUG: Fix some PeriodIndex resampling issues (pandas-dev#16153)
  BUG: Fix unexpected sort in groupby (pandas-dev#17621)
  DOC: Fixed typo in documentation for 'pandas.DataFrame.replace' (pandas-dev#17731)
  BUG: Fix series rename called with str altering name rather index (GH17407) (pandas-dev#17654)
  DOC: Add examples for MultiIndex.get_locs + cleanups (pandas-dev#17675)
  Doc improvements for IntervalIndex and Interval (pandas-dev#17714)
  BUG: DataFrame sort_values and multiple "by" columns fails to order NaT correctly
  Last of the timezones funcs (pandas-dev#17669)
  Add missing file to _pyxfiles, delete commented-out (pandas-dev#17712)
  update imports of DateParseError, remove unused imports from tslib (pandas-dev#17713)
  ...

@jschendel jschendel deleted the jschendel:depr-cdate_range branch Oct 2, 2017

alanbato added a commit to alanbato/pandas that referenced this pull request Nov 10, 2017

No-Stream added a commit to No-Stream/pandas that referenced this pull request Nov 28, 2017

@jreback jreback referenced this pull request Jun 30, 2019

Open

DEPR: deprecations log for removed issues #13777

141 of 141 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.