-
Notifications
You must be signed in to change notification settings - Fork 13
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
feat: ExpandDateTime convenience #15
feat: ExpandDateTime convenience #15
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.
Thanks for putting this in @sfc-gh-twhite !
Overall I think this is good implementation, and I am 100% ok with duplicating some code.
My main question throughout is whether it would be more convenient on the user side to not have to specify the date and time components separately? Given that this is a convenience method to combine the functionality of two other existing methods, I would be inclined to smash it all together so you don't have to pass in two separate lists of components to use it.
What do you think?
date_components: Sequence[ | ||
Literal["day", "week", "month", "year", "dow", "doy"] | ||
] = ( | ||
"dow", | ||
"month", | ||
"year", | ||
), | ||
time_components: Sequence[ |
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.
Is there value in separating these into date
and time
components? What if it were datetime_components
throughout?
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.
I agree! It would be worthwhile to combine the two rather than separating them. I can give it another try!
I adjusted the test to use ml.timestamp()
rather than the column name.
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.
Hey @sfc-gh-twhite ! Hope you had a nice break over the holidays. Did you want to take another crack at combining the date and time components into a single set?
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.
Hey @gforsyth, I do plan to get back to this! I've been spending more time with the core Ibis project, but I am jumping back to IbisML and can hopefully get something together soon.
37b3e48
to
2b69ad5
Compare
Closing as superseded by #20. |
Closes #14
Combined the functionality of these and added a test for it.
I didn't re-use the existing code in the implementation, so it may be worth a revisit to see if there is a preferred method. :)
Here's a quick snippet I wrote for testing!