DEPR: Deprecate cdate_range and merge into bdate_range by jschendel · Pull Request #17691 · pandas-dev/pandas (original) (raw)
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
- closes Merge cdate_range functionality into bdate_range #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
FutureWarningtocdate_rangeindicating deprecation- Added a test for this too
- Converted other tests using
cdate_rangeto usebdate_rangeinstead- Cleaned up the entire test script to make it more consistent with others (i.e.
withcontext)
- Cleaned up the entire test script to make it more consistent with others (i.e.
- Removed a reference to
cdate_rangefrom the current whatsnew (previous commit I wrote)
Expanded functionality of bdate_range:
- Added the additional
cdate_rangeparams (weekmask,holidays) for custom frequency ranges - Expanded that functionality beyond what was previously present in
cdate_rangecdate_rangeonly supported cday with theweekmaskandholidaysparams, 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
UserWarningif non-defaultweekmaskorholidaysare passed without a custom freq.- Added a test for this too.
Edits to timeseries.rst:
- Added a custom freq
bdate_rangeexample 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.).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks pretty good
| .. _timeseries.representation: |
|---|
| Time Stamps vs. Time Spans |
| Timestamps vs. Time Spans |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>`. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use origin
| pd.bdate_range(start=start, periods=20) |
| The start and end dates are strictly inclusive. So it will not generate any |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the appropriate location in this file
| - :func:`MultiIndex.is_monotonic_decreasing` has been implemented. Previously returned ``False`` in all cases. (:issue:`16554`) |
|---|
| - :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 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move to deprecation section, mention the deprecation, and include a ref
| - ``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`) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ' |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raise here, this is invalid
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this can raise.
| tm.assert_index_equal(result_2, expected_2) |
|---|
| class TestCustomDateRange(object): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deprecation
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this!
Added a few comments
| .. ipython:: python |
| pd.Timestamp(1349720105, unit='s') |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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', |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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' ?)
| 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``, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ' |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this can raise.
| rng : DatetimeIndex |
|---|
| """ |
| warnings.warn("cdate_range is deprecated and will be removed in a future " |
| "version, instead use bdate_range(..., freq='{freq}')" |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bdate_range -> pd.bdate_range (to make it clear that bdate_range is top-level, as opposed to cdate_range)
Made the review changes and fixed some unrelated typos I noticed in whatsnew (missing : and `).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
| pd.Timestamp.min |
|---|
| pd.Timestamp.max |
| See :ref:`here <timeseries.oob>` for ways to represent data outside these bound. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this to the top of the subsection. That way you can in the whatsnew refer directly to this section.
| 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`) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for fixing this!
| funcs = ['bdate_range', 'concat', 'crosstab', 'cut', |
|---|
| 'date_range', 'interval_range', 'eval', |
| 'factorize', 'get_dummies', 'cdate_range', |
| 'factorize', 'get_dummies', |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move to the deprecated functions section, and add a test (in same module), model after the top-level deprecations.
| FY5253, |
|---|
| FY5253Quarter, |
| ]) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you change this?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
k cool
Made the additional review changes.
| def test_deprecation_cdaterange(self): |
| # GH17596 |
| from pandas.core.indexes.datetimes import cdate_range |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, ok then; guess we moved this in 0.21.0, so this is fine then.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a minor comment.
ghost pushed a commit to reef-technologies/pandas that referenced this pull request
- '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) ...
alanbato pushed a commit to alanbato/pandas that referenced this pull request
No-Stream pushed a commit to No-Stream/pandas that referenced this pull request
