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

[REVIEW] duration type unary casts and binary ops #5394

Merged
merged 78 commits into from
Jul 22, 2020

Conversation

sriramch
Copy link
Contributor

@sriramch sriramch commented Jun 4, 2020

another part of #5272

  • this builds on [REVIEW] duration types #5359 and fixes or adds a few things
  • adds is_chrono and relevant chrono traits
  • support unary_casts for duration types
  • supports binops with chrono types (add/sub/mul/div/mod)
  • added tests for new unary casts with durations
  • include duration types in the typed tests
  • create factory methods for fixed_width_column_wrapper that can implicitly convert numeric types into target fixed width types

some of these items have already been pulled into #5359 and thus is a part of this pr (as #5359 isn't merged yet)

  • unifies the duration and timestamp scalars
  • reworked the duration type so that we can support conversions between durations that can't be supported by the language (lower_precision_duration = higher_precision_duration) (chronos duration types are used)

i have a left a few TODOs that will be addressed in follow-up pr's.

sriramch added 3 commits June 4, 2020 04:08
  - this builds on rapidsai#5359 and fixes or adds a few things
  - unifies the duration and timestamp scalars
  - adds `is_chrono` and relevant chrono traits
  - reworked the duration type so that we can support conversions between durations that
    can't be supported by the language(lower_precision_duration = higher_precision_duration)
  - include duration types in the typed tests
  - added tests for new unary casts with durations

i have left a few TODOs which will be in the follow-up pr's
@sriramch sriramch requested review from a team as code owners June 4, 2020 23:52
@harrism harrism mentioned this pull request Jun 4, 2020
15 tasks
Copy link
Contributor

@jrhemstad jrhemstad left a comment

Choose a reason for hiding this comment

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

reworked the duration type so that we can support conversions between durations that can't be supported by the language

Can you elaborate on this? I'm generally not supportive of sidestepping type rules that C++ has defined.

cpp/include/cudf/wrappers/durations.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/wrappers/timestamps.hpp Outdated Show resolved Hide resolved
@sriramch
Copy link
Contributor Author

sriramch commented Jun 5, 2020

Can you elaborate on this? I'm generally not supportive of sidestepping type rules that C++ has defined.

constructing a duration with a different duration (where the 2 duration types are different) requires that the period of the source duration is perfectly divisible by the time period of the target duration (if the tick counts are integral types, which is the case here). yet, the cudf timepoint (and the new duration types) allows this by performing a duration_cast of the source duration. i was wondering if there was a perceived need that was present earlier to allow these kinds of truncations to happen (i.e. there was a requirement to support something like timestamp_D = timestamp_s etc.), which is explicitly forbidden by std chrono.

cpp/include/cudf/scalar/scalar.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/scalar/scalar.hpp Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 8, 2020

Codecov Report

Merging #5394 into branch-0.15 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           branch-0.15    #5394   +/-   ##
============================================
  Coverage        86.38%   86.38%           
============================================
  Files               76       76           
  Lines            13033    13033           
============================================
  Hits             11258    11258           
  Misses            1775     1775           

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 79bd435...b7de3c9. Read the comment docs.

  - for now, it supports product of duration with tick counts
@sriramch sriramch requested a review from a team as a code owner June 9, 2020 17:57
sriramch added 3 commits June 9, 2020 23:48
  - this can also prevent implicit downcasts and truncations
  - one can still opt for it explicitly with unary casts
cpp/tests/utilities/column_wrapper.hpp Outdated Show resolved Hide resolved
cpp/tests/utilities/column_wrapper.hpp Outdated Show resolved Hide resolved
@sriramch sriramch requested a review from jrhemstad July 13, 2020 20:59
@harrism
Copy link
Member

harrism commented Jul 17, 2020

rerun tests

  - this was required in 10.0 builds due to nvcc issues
  - now that 10.0 builds have been removed, remove the specialization as well
@sriramch sriramch requested a review from jrhemstad July 17, 2020 20:12
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 4 - Needs Review Waiting for reviewer to review or respond libcudf Affects libcudf (C++/CUDA) code. Spark Functionality that helps Spark RAPIDS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants