-
-
Notifications
You must be signed in to change notification settings - Fork 18k
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 box kwarg for to_timedelta and to_datetime #24486
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you will have to catch the warning in tests that currently use box
You mean assert that the warning is raised in those cases? |
yes need to eliminate all warnings from the test suite - either catch them or remove the box parameter (prob the former is ok here) |
doc/source/whatsnew/v0.24.0.rst
Outdated
@@ -1152,6 +1152,7 @@ Deprecations | |||
- Passing a string alias like ``'datetime64[ns, UTC]'`` as the ``unit`` parameter to :class:`DatetimeTZDtype` is deprecated. Use :class:`DatetimeTZDtype.construct_from_string` instead (:issue:`23990`). | |||
- In :meth:`Series.where` with Categorical data, providing an ``other`` that is not present in the categories is deprecated. Convert the categorical to a different dtype or add the ``other`` to the categories first (:issue:`24077`). | |||
- :meth:`Series.clip_lower`, :meth:`Series.clip_upper`, :meth:`DataFrame.clip_lower` and :meth:`DataFrame.clip_upper` are deprecated and will be removed in a future version. Use ``Series.clip(lower=threshold)``, ``Series.clip(upper=threshold)`` and the equivalent ``DataFrame`` methods (:issue:`24203`) | |||
- The functions :func:`pandas.to_datetime` and :func:`pandas.to_timedelta` have deprecated the ``box`` keyword. Use :attr:`Series.values` and :meth:`Timestamp.to_datetime64`/:meth:`Timedelta.to_timedelta64` instead to get an ndarray of values or ``numpy.timestamp64``/``numpy.timedelta64``, respectively (:issue:`24416`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recommend .to_numpy()
pandas/core/tools/timedeltas.py
Outdated
@@ -40,6 +43,12 @@ def to_timedelta(arg, unit='ns', box=True, errors='raise'): | |||
- If True returns a Timedelta/TimedeltaIndex of the results. | |||
- If False returns a numpy.timedelta64 or numpy.darray of | |||
values of dtype timedelta64[ns]. | |||
|
|||
.. deprecated:: 0.24.0 | |||
Use :attr:`Series.values` or :meth:`Timedelta.to_timedelta64` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here as well
doc/source/whatsnew/v0.24.0.rst
Outdated
@@ -1152,6 +1152,7 @@ Deprecations | |||
- Passing a string alias like ``'datetime64[ns, UTC]'`` as the ``unit`` parameter to :class:`DatetimeTZDtype` is deprecated. Use :class:`DatetimeTZDtype.construct_from_string` instead (:issue:`23990`). | |||
- In :meth:`Series.where` with Categorical data, providing an ``other`` that is not present in the categories is deprecated. Convert the categorical to a different dtype or add the ``other`` to the categories first (:issue:`24077`). | |||
- :meth:`Series.clip_lower`, :meth:`Series.clip_upper`, :meth:`DataFrame.clip_lower` and :meth:`DataFrame.clip_upper` are deprecated and will be removed in a future version. Use ``Series.clip(lower=threshold)``, ``Series.clip(upper=threshold)`` and the equivalent ``DataFrame`` methods (:issue:`24203`) | |||
- The functions :func:`pandas.to_datetime` and :func:`pandas.to_timedelta` have deprecated the ``box`` keyword. Use :attr:`Series.values` and :meth:`Timestamp.to_datetime64`/:meth:`Timedelta.to_timedelta64` instead to get an ndarray of values or ``numpy.timestamp64``/``numpy.timedelta64``, respectively (:issue:`24416`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
numpy.timestamp64
--> numpy.datetime64
Codecov Report
@@ Coverage Diff @@
## master #24486 +/- ##
===========================================
- Coverage 92.31% 43.05% -49.26%
===========================================
Files 166 166
Lines 52335 52339 +4
===========================================
- Hits 48313 22535 -25778
- Misses 4022 29804 +25782
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #24486 +/- ##
==========================================
- Coverage 91.28% 91.27% -0.01%
==========================================
Files 173 173
Lines 52965 52969 +4
==========================================
+ Hits 48348 48350 +2
- Misses 4617 4619 +2
Continue to review full report at Codecov.
|
Would it make sense to transition all of the internal uses of box here as well? |
yep. |
pandas/io/parsers.py
Outdated
dayfirst=dayfirst, | ||
errors='ignore', | ||
infer_datetime_format=infer_datetime_format | ||
) | ||
).to_pydatetime() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a very lower performance option, rather just do
tools.to_datetime(.....).to_numpy()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
though you may actually be able to get away with not doing any conversion and just remove the box=, see if it passes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I had considered the latter, but wasn't sure whether there would be other consequences. I'll give it a try and see if it passes. The test fails with .to_numpy()
because we lose the tzinfo.
result = pd.to_datetime(dates, format=fmt, box=box) | ||
expected = const(expected_dates) | ||
tm.assert_equal(result, expected) | ||
with catch_warnings(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't do this,
use tm.assert_produces_warning
result = to_datetime(arr, box=True) | ||
assert result is arr | ||
with catch_warnings(): | ||
simplefilter("ignore", FutureWarning) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for the rest (in this case you can just remove the keyword entirely)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Will do.
Hello @cbertinato! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2019-03-12 15:23:12 UTC |
some lint issues and needs a rebase |
pandas/core/indexes/datetimelike.py
Outdated
@@ -232,7 +232,12 @@ def asobject(self): | |||
return self.astype(object) | |||
|
|||
def _convert_tolerance(self, tolerance, target): | |||
tolerance = np.asarray(to_timedelta(tolerance, box=False)) | |||
tolerance = to_timedelta(tolerance) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see #24653 where this could be much simpler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally would do that as a pre-cursor PR
pandas/io/parsers.py
Outdated
@@ -3120,18 +3122,28 @@ def _get_lines(self, rows=None): | |||
def _make_date_converter(date_parser=None, dayfirst=False, | |||
infer_datetime_format=False): | |||
def converter(*date_cols): | |||
from pandas.core.dtypes.common import is_datetime64_dtype |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be imported at the top
errors='raise') | ||
# Assuming all datetimes are in bounds, to_datetime() returns | ||
# an array that is equal to Timestamp() parsing | ||
tm.assert_numpy_array_equal( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can just remove the box arg here
# A list of datetimes where the last one is out of bounds | ||
dts_with_oob = dts + [np.datetime64('9999-01-01')] | ||
|
||
pytest.raises(ValueError, pd.to_datetime, dts_with_oob, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the form
with pytest.raises(...):
...
pytest.raises(ValueError, pd.to_datetime, dts_with_oob, | ||
errors='raise') | ||
|
||
with tm.assert_produces_warning(FutureWarning): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove box arg
[dt.item() for dt in dts_with_oob], | ||
dtype='O' | ||
|
||
with tm.assert_produces_warning(FutureWarning): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove box arg
51be01f
to
2f81938
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should do #24653 as a pre-cursor here to make this simpler
d62170f
to
ab05d74
Compare
pandas/core/tools/datetimes.py
Outdated
@@ -444,6 +447,12 @@ def to_datetime(arg, errors='raise', dayfirst=False, yearfirst=False, | |||
|
|||
- If True returns a DatetimeIndex or Index-like object | |||
- If False returns ndarray of values. | |||
|
|||
.. deprecated:: 0.24.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0.25
pandas/core/tools/timedeltas.py
Outdated
@@ -40,6 +43,12 @@ def to_timedelta(arg, unit='ns', box=True, errors='raise'): | |||
- If True returns a Timedelta/TimedeltaIndex of the results. | |||
- If False returns a numpy.timedelta64 or numpy.darray of | |||
values of dtype timedelta64[ns]. | |||
|
|||
.. deprecated:: 0.24.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0.25
ab05d74
to
f388126
Compare
@cbertinato this is looking close, can you see if get passing |
63717f5
to
427345d
Compare
All green! |
doc/source/whatsnew/v0.25.0.rst
Outdated
@@ -94,6 +94,7 @@ Deprecations | |||
~~~~~~~~~~~~ | |||
|
|||
- Deprecated the `M (months)` and `Y (year)` `units` parameter of :func: `pandas.to_timedelta`, :func: `pandas.Timedelta` and :func: `pandas.TimedeltaIndex` (:issue:`16344`) | |||
- The functions :func:`pandas.to_datetime` and :func:`pandas.to_timedelta` have deprecated the ``box`` keyword. Use :attr:`Series.values` and :meth:`Timestamp.to_datetime64`/:meth:`Timedelta.to_timedelta64` instead to get an ndarray of values or ``numpy.timestamp64``/``numpy.timedelta64``, respectively (:issue:`24416`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should update .values
to .to_numpy()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes indeed. Updated.
427345d
to
7893630
Compare
7893630
to
4d6e0d3
Compare
All green. |
thanks @cbertinato nice patch! |
* master: (22 commits) Fixturize tests/frame/test_operators.py (pandas-dev#25641) Update ValueError message in corr (pandas-dev#25729) DOC: fix some grammar and inconsistency issues in the User Guide (pandas-dev#25728) ENH: Add public start, stop, and step attributes to RangeIndex (pandas-dev#25720) Make Rolling.apply documentation clearer (pandas-dev#25712) pandas-dev#25707 - Fixed flakiness in stata write test (pandas-dev#25714) Json normalize nan support (pandas-dev#25619) TST: resolve issues with test_constructor_dtype_datetime64 (pandas-dev#24868) DEPR: Deprecate box kwarg for to_timedelta and to_datetime (pandas-dev#24486) BUG: Preserve name in DatetimeIndex.snap (pandas-dev#25585) Fix concat not respecting order of OrderedDict (pandas-dev#25224) CLN: remove pandas.core.categorical (pandas-dev#25655) TST/CLN: Remove more Panel tests (pandas-dev#25675) Pinned pycodestyle (pandas-dev#25701) DOC: update date of 0.24.2 release notes (pandas-dev#25699) BUG: Fix error in replace with strings that are large numbers (pandas-dev#25616) (pandas-dev#25644) BUG: fix usage of na_sentinel with sort=True in factorize() (pandas-dev#25592) BUG: Fix to_string output when using header (pandas-dev#16718) (pandas-dev#25602) CLN: Remove unused test code (pandas-dev#25670) CLN: remove Panel from concat error message (pandas-dev#25676) ... # Conflicts: # doc/source/whatsnew/v0.25.0.rst
git diff upstream/master -u -- "*.py" | flake8 --diff
There are still some tests that are failing on Travis, but they appear to be unrelated. I'll see what CI returns for the PR and address any tests that continue to fail that might be due to the changes here.