-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
DOC: seconds attribute of timedelta surprises and no warning is given in the docs #50969
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
Comments
Thanks @Styrke for the report Isn't this correct? It says
and that's what's been returned:
This isn't really possible as it would end up displaying a warning each time some calls it on purpose I think this just needs an example. Do you want to open a pull request to add this an example? |
Thanks for your reply @MarcoGorelli.
It is technically correct but the pitfall is in what it doesn't say and the apparent interaction with negative Timedeltas. First, you have to be quite alert when reading that description to consciously notice that it excludes days, months, and years. Second, even if I had read and fully realised the implications of that description, I would have disregarded it because I am working with timedeltas of up to a few hours at most. I did not realise that -1 minute would be represented as -1 days +23:59:00, thus putting me in a situation where I am affected by the limits of the
I may display some ignorance in the following because I haven't looked at the source code. Please just let me know if I have any substantial misunderstandings. I can't imagine what need downstream users of pandas would have that they should prefer the My guess is that the I think that the appropriate action here would be to make it very clear to everyone that they probably want to use the Ideally there would also be a warning that is displayed every time the
If you agree, I will take a stab at both updating the description of the |
Sure, would take a doc clarification, thanks! |
take |
@MarcoGorelli I've worked on this issue over the past 2 weeks and I can't figure out which file I need up update the docstrings in. After I build my dev environment I see that a .c file is generated that contains all of the docstrings for the Timedelta class. However, that doesn't appear to be the file that I should actually be updating. Could you provide some guidance on this issue? Thanks! |
Hey, what do you mean by
|
@MarcoGorelli I initially thought that the C file was the correct file to update. However, I don't see any .c files in the pandas GitHub repo. Git also warned me that I probably shouldn't push the .c file to Github. I'm assuming that I'm probably updating the wrong file. Sorry for the confusion. This is my first time contributing to pandas! |
which .c file did you try editing? |
It was I was updating the comments in that file and then I would re-package the pandas lib and run help() in a python interactive interpreter to confirm my changes |
ah you probably want the corresponding |
So, I thought so too. The pyx file is linked to the C file via the "cythonization" that takes place when I create the build environment right? My problem (or lack of understanding) is that whenever I update the c file, re-build by build environment and then run git status no files appear to have been modified. I also don't see any doc-strings in the .pyx file either. |
You don't update the c file, you update the pyx file Here's the docstring pandas/pandas/_libs/tslibs/timedeltas.pyx Lines 1061 to 1092 in d0fbeb4
|
ah okay! Thank you for the clarity! Much appreciated! I'll take another stab at this issue later tonight! |
take |
* Updating the timedeltas.seconds docstring for Issue #50969 * Updating the docstring for Timedelta.seconds * Removing docstrings on lines 1068-1071 as they're not needed. * Update pandas/_libs/tslibs/timedeltas.pyx --------- Co-authored-by: Marco Edward Gorelli <[email protected]>
Pandas version checks
main
hereLocation of the documentation
https://pandas.pydata.org/docs/dev/reference/api/pandas.Timedelta.html?highlight=timedelta#pandas.Timedelta
Documentation problem
I think the documentation for the
seconds
attribute of a Timedelta object should have a clear warning to tell people that they probably want to use thetotal_seconds()
method instead.Apparently, the
seconds
attribute of a Timedelta object takes hours, minutes, and seconds into account, but not days which I wouldn't even imagine would be necessary in the example below where I try to find the difference in seconds between two datetimes that are a minute apart:This is really surprising to me. Only later did I realise that the
total_seconds()
method exists. Apparently it does what I expected from theseconds
attribute in the first place.Suggested fix for documentation
In my case, of course, the ideal would have been if pandas had displayed a warning when I use the
seconds
attribute. Or even if it just did what I expected.Assuming that it is for better or for worse the intended behaviour, shouldn't it be better documented? Is there a way that I could have known this? Is it described in some place that I didn't know to look?
The text was updated successfully, but these errors were encountered: