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

Support additional format specifiers in from_timestamps #9047

Merged

Conversation

davidwendt
Copy link
Contributor

Reference #5991

This PR adds support for the following format specifiers in cudf::strings::from_timestamp

%a and %A -- weekday names (passed into the API)
%b and %B -- month names (passed into the API)
%u - ISO weekday (1-7)
%w - weekday (0-6)
%U - week of the year (Sunday based)
%W - week of the year (Monday based)
%V - ISO week of the year
%G - Year based on ISO weeks

This adds a new parameter to the API for the caller to pass then string names for the weekdays and months. These are only required if the %a, %b, %A, %B specifiers are contained in the format string.

The change to from_timestamps is mainly a rewrite to include logic for these specifiers. Some common code required corresponding changes to to_timestamps and is_timestamps though these functions have not changed in this PR.

@davidwendt davidwendt added 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. strings strings issues (C++ and Python) improvement Improvement / enhancement to an existing function breaking Breaking change labels Aug 16, 2021
@davidwendt davidwendt self-assigned this Aug 16, 2021
@davidwendt davidwendt added this to PR-WIP in v21.10 Release via automation Aug 16, 2021
@codecov
Copy link

codecov bot commented Aug 16, 2021

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.10@e222584). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 7aafbd5 differs from pull request most recent head e6d4cd1. Consider uploading reports for the commit e6d4cd1 to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.10    #9047   +/-   ##
===============================================
  Coverage                ?   10.82%           
===============================================
  Files                   ?      114           
  Lines                   ?    18709           
  Branches                ?        0           
===============================================
  Hits                    ?     2026           
  Misses                  ?    16683           
  Partials                ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e222584...e6d4cd1. Read the comment docs.

@davidwendt davidwendt added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Aug 24, 2021
@davidwendt davidwendt moved this from PR-WIP to PR-Needs review in v21.10 Release Aug 24, 2021
@davidwendt davidwendt marked this pull request as ready for review August 24, 2021 14:52
@davidwendt davidwendt requested a review from a team as a code owner August 24, 2021 14:52
@davidwendt davidwendt added this to the Time Series Analysis milestone Aug 25, 2021
Copy link
Contributor

@devavret devavret left a comment

Choose a reason for hiding this comment

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

LGTM

cpp/include/cudf/strings/convert/convert_datetime.hpp Outdated Show resolved Hide resolved
cpp/src/strings/convert/convert_datetime.cu Outdated Show resolved Hide resolved
v21.10 Release automation moved this from PR-Needs review to PR-Reviewer approved Aug 27, 2021
Copy link
Contributor

@codereport codereport left a comment

Choose a reason for hiding this comment

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

Looks really good!

cpp/include/cudf/strings/convert/convert_datetime.hpp Outdated Show resolved Hide resolved
@davidwendt
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit fdcb90a into rapidsai:branch-21.10 Aug 31, 2021
v21.10 Release automation moved this from PR-Reviewer approved to Done Aug 31, 2021
@davidwendt davidwendt deleted the fea-more-timestamp-fmt-specifiers branch August 31, 2021 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team breaking Breaking change improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. strings strings issues (C++ and Python)
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants