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

BUG: Either incorrect unit validation for 'T' in to_timedelta() or incorrect documentation #52536

Closed
3 tasks done
juliangall opened this issue Apr 8, 2023 · 13 comments · Fixed by #53557
Closed
3 tasks done
Labels
Bug good first issue Typing type annotations, mypy/pyright type checking

Comments

@juliangall
Copy link

Pandas version checks

  • I have checked that this issue has not already been reported.

  • I have confirmed this bug exists on the latest version of pandas.

  • I have confirmed this bug exists on the main branch of pandas.

Reproducible Example

import pandas as pd
td = pd.to_timedelta(1, unit='t')  # success
td = pd.to_timedelta(1, unit='T')  # runs ok but fails type-checking in IDE

Issue Description

The specification of the unit parameter for to_timedelta() in https://github.com/pandas-dev/pandas/tree/v2.0.0/pandas/core/tools/timedeltas.py is as follows:

unit: UnitChoices | None = None

The documentation for to_timedelta() (and the comment in the code) allows for (among others):

‘m’ / ‘minute’ / ‘min’ / ‘minutes’ / ‘T’

The allowed values in UnitChoices (https://github.com/pandas-dev/pandas/tree/main/pandas/_libs/tslibs/timedeltas.pyi) include 't', but not 'T'.

Expected Behavior

It seems to me that it is usual to allow upper case 'T' as a specifier for minute, but others may know better.

Either 'T' should be allowed in UnitChoices, or the to_timedelta() documentation should be updated to specify lower case 't'

Installed Versions

INSTALLED VERSIONS

commit : 478d340
python : 3.9.6.final.0
python-bits : 64
OS : Darwin
OS-release : 22.4.0
Version : Darwin Kernel Version 22.4.0: Mon Mar 6 21:00:17 PST 2023; root:xnu-8796.101.5~3/RELEASE_X86_64
machine : x86_64
processor : i386
byteorder : little
LC_ALL : None
LANG : None
LOCALE : en_GB.UTF-8
pandas : 2.0.0
numpy : 1.24.2
pytz : 2023.3
dateutil : 2.8.2
setuptools : 65.5.1
pip : 22.3.1
Cython : None
pytest : None
hypothesis : None
sphinx : None
blosc : None
feather : None
xlsxwriter : None
lxml.etree : None
html5lib : None
pymysql : None
psycopg2 : None
jinja2 : None
IPython : None
pandas_datareader: None
bs4 : None
bottleneck : None
brotli : None
fastparquet : None
fsspec : None
gcsfs : None
matplotlib : 3.7.1
numba : None
numexpr : None
odfpy : None
openpyxl : None
pandas_gbq : None
pyarrow : None
pyreadstat : None
pyxlsb : None
s3fs : None
scipy : 1.10.1
snappy : None
sqlalchemy : 1.4.47
tables : None
tabulate : None
xarray : None
xlrd : None
zstandard : None
tzdata : 2023.3
qtpy : None
pyqt5 : None

@juliangall juliangall added Bug Needs Triage Issue that has not been reviewed by a pandas team member labels Apr 8, 2023
@topper-123
Copy link
Contributor

If you look down the code path, you'll see that the code will be converted to lower case:

cpdef inline str parse_timedelta_unit(str unit):
"""
Parameters
----------
unit : str or None
Returns
-------
str
Canonical unit string.
Raises
------
ValueError : on non-parseable input
"""
if unit is None:
return "ns"
elif unit == "M":
return unit
try:
return timedelta_abbrevs[unit.lower()]
except KeyError:
raise ValueError(f"invalid unit abbreviation: {unit}")

So code-wise, the choice of case won't matter.

Now if you look into the type definition, you'll only see lowercase t, i.e. upper case T will fail.

UnitChoices = Literal[
"Y",
"y",
"M",
"W",
"w",
"D",
"d",
"days",
"day",
"hours",
"hour",
"hr",
"h",
"m",
"minute",
"min",
"minutes",
"t",
"s",
"seconds",
"sec",
"second",
"ms",
"milliseconds",
"millisecond",
"milli",
"millis",
"l",
"us",
"microseconds",
"microsecond",
"µs",
"micro",
"micros",
"u",
"ns",
"nanoseconds",
"nano",
"nanos",
"nanosecond",
"n",
]

Upper case T should probably be in the UnitChoices list. A PR is welcome.

@topper-123 topper-123 added Typing type annotations, mypy/pyright type checking good first issue and removed Needs Triage Issue that has not been reviewed by a pandas team member labels May 20, 2023
@ezqdavid
Copy link

Hi, i would like to take this issue if i can. It would be my first PR in an open source project.

@ezqdavid
Copy link

take

@topper-123
Copy link
Contributor

👍

ezqdavid added a commit to ezqdavid/pandas that referenced this issue May 24, 2023
@natmokval
Copy link
Contributor

natmokval commented Jun 7, 2023

Hi @ezqdavid, I'd like to create a pr for this issue if you don't mind.

@jbrockmendel
Copy link
Member

T and l seem really weird here. i think we should deprecate them.

@ezqdavid
Copy link

ezqdavid commented Jun 8, 2023

Hi @natmokval . Yes please, i dont mind. I was unsure of making the pr. Thank you.

@natmokval
Copy link
Contributor

natmokval commented Jun 8, 2023

T and l seem really weird here. i think we should deprecate them.

Thank you, @jbrockmendel for the comment. I will update the documentation for to_timedelta and remove "T" from the list of units. I think I need to correct the method parse_timedelta_unit and raise a deprecation warning in case a user tries to pass "T". I am not sure If we deprecate “T” for minutes, should we deprecate “t” as well?

I have another question. Do you mean by “l” milliseconds?

@jbrockmendel
Copy link
Member

Do you mean by “l” milliseconds?

Correct.

If we deprecate “T” for minutes, should we deprecate “t” as well?

I think so, yes.

@topper-123
Copy link
Contributor

+1 from me. The upper and lower t are synomous, so both can be deprecated.

@anttimc
Copy link

anttimc commented Aug 14, 2024

I haven't figured how 'H' for hour got deprecated after issue?

I'd just like to note that the deprecations in pandas cause huge amount of work in dependent projects.

@MarcoGorelli
Copy link
Member

hi @anttimc - I'll find the issue, looks like the referenced issue was incorrect

I'd just like to note that the deprecations in pandas cause huge amount of work in dependent projects.

sorry about that - which project are you referring to? if you use 'h', it should work across old and new pandas versions

@MarcoGorelli
Copy link
Member

MarcoGorelli commented Aug 14, 2024

For 'H' in particular, it looks like I'd suggested it here - #54061 (comment). There probably should have been more discussion to be fair, although it did seem in-line with the other renamings suggested here

With #55502 in, the rule would then be that sub-daily frequencies (Ticks, fixed frequencies) are all lower-case, whereas daily and above upper-case

The issue with allowing case-insensitivity is that then this would happen:

In [6]: pandas.date_range('2000', periods=3, freq='NS')  # nanosecond, cool
Out[6]:
DatetimeIndex([          '2000-01-01 00:00:00',
               '2000-01-01 00:00:00.000000001',
               '2000-01-01 00:00:00.000000002'],
              dtype='datetime64[ns]', freq='N')

In [7]: pandas.date_range('2000', periods=3, freq='US')  # microsecond, cool
Out[7]:
DatetimeIndex([       '2000-01-01 00:00:00', '2000-01-01 00:00:00.000001',
               '2000-01-01 00:00:00.000002'],
              dtype='datetime64[ns]', freq='U')

In [8]: pandas.date_range('2000', periods=3, freq='MS')  # wait, this isn't millisecond?
Out[8]: DatetimeIndex(['2000-01-01', '2000-02-01', '2000-03-01'], dtype='datetime64[ns]', freq='MS')

An alternative would have been to keep case-insensitivity, but rename 'MS' to MB', but that got pushback too #56840

Correcting historic issues is tricky - we either leaves things as they are and leave inconsistencies and issues around, or deprecate towards more consistent and predictable behaviour

I understand that this one is a pain for people building on top of pandas, but I thought a simple find-and-replace would be enough here - if that's not the case, please do report an issue, it's a sign that something may not be right. Thanks 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug good first issue Typing type annotations, mypy/pyright type checking
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants