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

DOC: Add notes about M and Y to to_timedelata documentation. (#34968) #34979

Merged
merged 3 commits into from
Sep 26, 2020
Merged

DOC: Add notes about M and Y to to_timedelata documentation. (#34968) #34979

merged 3 commits into from
Sep 26, 2020

Conversation

selasley
Copy link
Contributor

@selasley selasley commented Jun 24, 2020

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

Not sure it closes these issues, but using "M" in the arg argument of to_timedelta caused confusion in #34968 and #27285. #33094 is somewhat related. I came across another issue I can't find now where there was confusion about "Y" returning a days with times delta

The data to be converted to timedelta. Note that the character M
is treated as minute, not month. The characters Y and y are treated
as the mean length of the Gregorian calendar year - 365.2425 days or
365 days 5 hours 49 minutes 12 seconds
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
365 days 5 hours 49 minutes 12 seconds
365 days 5 hours 49 minutes 12 seconds.

@dsaxton dsaxton added the Docs label Jun 24, 2020
@dsaxton
Copy link
Member

dsaxton commented Jun 25, 2020

Can you also link to the relevant issue in the description?

@selasley
Copy link
Contributor Author

Since MS is valid for milliseconds and contains the character M my PR should probably be changed to something like

The data to be converted to timedelta. The character M by itself, e.g. '1M', is treated as minute, not month. The characters Y and y are treated as the mean length of the Gregorian calendar year - 365.2425 days or 365 days 5 hours 49 minutes 12 seconds.

@MarcoGorelli
Copy link
Member

Since MS is valid for milliseconds and contains the character M my PR should probably be changed to something like

The data to be converted to timedelta. The character M by itself, e.g. '1M', is treated as minute, not month. The characters Y and y are treated as the mean length of the Gregorian calendar year - 365.2425 days or 365 days 5 hours 49 minutes 12 seconds.

Makes sense to me - do you want to push such an update?

@pep8speaks
Copy link

pep8speaks commented Sep 1, 2020

Hello @selasley! 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 2020-09-01 16:05:04 UTC

@selasley
Copy link
Contributor Author

selasley commented Sep 1, 2020

I think I updated it correctly ( https://xkcd.com/1597/ ). If not, please let me know what I should have done to update the PR. Thanks.

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

Pending green, this looks correct to me

@TomAugspurger
Copy link
Contributor

Does this close #34968? If not, can you suggest what needs to be done there?

@MarcoGorelli
Copy link
Member

As far as I can tell, this closes #34968 - @selasley , seeing as you wrote "Not sure it closes these issues", do you agree?

@selasley
Copy link
Contributor Author

Yes, I believe documenting the current behavior addresses the issue. Thanks.

@MarcoGorelli
Copy link
Member

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 1 pipeline(s).

@MarcoGorelli
Copy link
Member

>>> (pd.Timestamp.today() + pd.to_timedelta('1 Y')) - pd.Timestamp.today()
Timedelta('365 days 05:49:11.999916')
>>> (pd.Timestamp.today() + pd.to_timedelta('1 M')) - pd.Timestamp.today()
Timedelta('0 days 00:00:59.999766')

Looks right to me...thanks @selasley !

@MarcoGorelli MarcoGorelli merged commit 455f34d into pandas-dev:master Sep 26, 2020
@MarcoGorelli MarcoGorelli added this to the 1.2 milestone Sep 26, 2020
@jreback
Copy link
Contributor

jreback commented Sep 26, 2020

hmm
did we not fully deprecate M and Y for timedelta conversion? i don't remember if these were actually removed

this added note is very misleading then

if the above is true then pls revert this

@MarcoGorelli
Copy link
Member

MarcoGorelli commented Sep 26, 2020

@jreback pd.to_timedelta(1, unit='M') is deprecated, but not pd.to_timedelta('1 M').

In issue #34968 you'd commented

I'll repurpose this issue to be a doc one if you'd like to submit a PR to update.

@jreback
Copy link
Contributor

jreback commented Sep 26, 2020

i c

pls create a new issue to deprecate these as strings as well

we do not support these as they are very confusing

yes they work but they are fixed offsets and almost never what you want

@MarcoGorelli
Copy link
Member

sure, done - #36666

kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020
…dev#34968) (pandas-dev#34979)

* DOC: Add notes about M and Y to to_timedelata documentation.  (pandas-dev#34968)

* DOC: Update notes about M and Y to to_timedelata documentation.  (pandas-dev#34968)

* DOC: Add notes about M and Y to to_timedelata documentation.  (pandas-dev#34968)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants