Skip to content

ENH: implement pandas_timedelta_to_timedeltastruct for other resos #46465

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 31, 2022
Merged

ENH: implement pandas_timedelta_to_timedeltastruct for other resos #46465

merged 3 commits into from
Mar 31, 2022

Conversation

jbrockmendel
Copy link
Member

  • closes #xxxx (Replace xxxx with the Github issue number)
  • Tests added and passed if fixing a bug or adding a new feature
  • All code checks passed.
  • Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

cc @WillAyd IIRC you implemented the original here. The new cases this implements are mostly copy/paste, so it wouldn't surprise me if there is a more elegant alternative. Thoughts?

WillAyd
WillAyd reviewed Mar 22, 2022
View reviewed changes
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

Nice! Glad to see you venture down the C path. This is a great start.

On first pass seems reasonable - your plan is to ultimately generate these for all precisions right? If so you might want to opt for a macro'ed approach similar to what's in khash.h rather than copy / pasting these.

Here's an example of what that might look like

pandas/pandas/_libs/src/klib/khash.h

Line 708 in 5414a2d

KHASH_MAP_INIT_STR(str, size_t)

pandas/_libs/tslibs/src/datetime/np_datetime.c Outdated
@@ -682,19 +682,23 @@ void pandas_timedelta_to_timedeltastruct(npy_timedelta td,
npy_int64 sfrac;
npy_int64 ifrac;
int sign;
npy_int64 DAY_NS = 86400000000000LL;
npy_int64 PER_DAY;
Copy link
Member

Choose a reason for hiding this comment

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

Just as a matter of convention should probably be lowercase. Usually you uppercase macros and constants, so on first read looking at these in the body of the functions a reader may mistakenly think that (DAY_NS was incorrect)

Copy link
Member Author

Choose a reason for hiding this comment

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

good to know, will update

@jbrockmendel
Copy link
Member Author

If so you might want to opt for a macro'ed approach similar to what's in khash.h rather than copy / pasting these.

my thought was to follow the pattern for the dt64 code in the same file, which would probably mean refactoring out some helper functions but not using macros. Is getting comfortable with macros the next step of leveling up my C-fu?

@WillAyd
Copy link
Member

WillAyd commented Mar 22, 2022

The macros would definitely be more advanced, at least as far as I understand. I am by no means an expert and not in a good enough position to teach, but from my personal experience the biggest things I've found to help grasp C are:

  • Treating every warning as an error. -Wall, -Wextra and -Werror are great switches; while C has some historic cruft in what the language will allow, most compilers do a great job of warning you about things that may be technically valid but not what you want
  • Getting comfortable with the debugger. gdb and lldb are great tools, and aren't a huge leap from pdb. The only thing to be aware of here is that if you want to debug a library you will also want to compile that library with debugging enabled, otherwise a lot of the function names, symbols and bodies will be stripped down to assembly

https://pandas.pydata.org/pandas-docs/stable/development/debugging_extensions.html might also help

@jbrockmendel jbrockmendel marked this pull request as ready for review March 23, 2022 21:08
@jbrockmendel
Copy link
Member Author

Thanks. Any objection to merging as-is? We may implement more units later, but this gets all the ones we're definitely doing.

@jbrockmendel
Copy link
Member Author

i guess it would make sense to have a corresponding function to go from tdstruct to td64. was that just not necessary at the time when it was nanos-only?

@WillAyd
Copy link
Member

WillAyd commented Mar 29, 2022

None from me. My feedback is just long term consideration but I think it’s great to move forward with what’s there and consolidate later

@mroeschke mroeschke added the Timedelta Timedelta data type label Mar 31, 2022
@mroeschke mroeschke added this to the 1.5 milestone Mar 31, 2022
mroeschke
mroeschke approved these changes Mar 31, 2022
View reviewed changes
Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

Not user facing necessarily right?

@jbrockmendel
Copy link
Member Author

Not user facing necessarily right?

nope, just needed to implement non-nano Timedelta

@mroeschke mroeschke merged commit 672798e into pandas-dev:main Mar 31, 2022
@jbrockmendel jbrockmendel deleted the enh-tdstruct branch March 31, 2022 02:25
yehoshuadimarsky pushed a commit to yehoshuadimarsky/pandas that referenced this pull request Jul 13, 2022
@jbrockmendel @yehoshuadimarsky
ENH: implement pandas_timedelta_to_timedeltastruct for other resos (p…
17cb27e 
…andas-dev#46465)

* ENH: implement pandas_timedelta_to_timedeltastruct for other resos

* idiomatic naming
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WillAyd WillAyd WillAyd left review comments

@mroeschke mroeschke mroeschke approved these changes

Assignees
No one assigned
Labels
Timedelta Timedelta data type
Projects
None yet
Milestone
1.5
Development

Successfully merging this pull request may close these issues.
3 participants
@jbrockmendel @WillAyd @mroeschke