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.

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

[REVIEW] fixed_point Column Optimization (store scale in data_type) [skip ci] #5861

Merged
merged 178 commits into from
Oct 13, 2020
Merged

[REVIEW] fixed_point Column Optimization (store scale in data_type) [skip ci] #5861

merged 178 commits into from
Oct 13, 2020

Conversation

codereport
Copy link
Contributor

@codereport codereport commented Aug 6, 2020 •
edited
Loading

This address a part of #3556 and is the follow up to Initial fixed_point Column Support #5704

The major steps to this PR are:

  • Add scale into data_type and scale() method
  • Hook up scale from data_type (set from fixed_point::_scale)
    • Merge ColumnLike (test-fixed-point-iterator) branch into this branch
    • Fuse ColumnLike and fixed_point
  • Customize allocation factory functions (to only allocate for underlying value)
  • Specialize algorithms so that they don't need to operate on full fixed_point type (i.e sort)
  • Add initial fixed_point_column_wrapper
  • Add scale_type to the make_numeric_column factory functions (new ctor for data_type)
  • Unit tests for ColumnLike mock up working (eventually to be refactored)
  • Add column_device_view::element methods for fixed_point
  • Add thrust::optional to data_type::_scale (removed)
  • Fix failing unit tests
  • Get fixed_point_column_wrapper working with data_type::_scale
  • Remove all TODOs
  • Resolve AST PR CI Failure

trevorsm7 and others added 21 commits May 22, 2020 18:49
@trevorsm7
Add fixed_point iterator test
704ed80
@trevorsm7
Add test demonstrating binary op
787876a
@trevorsm7
Replace cout with asserts
de6921b
@trevorsm7
Test with fraction fixed_point values
4046257
@trevorsm7
Attempt to implement proxy reference, but thrust doesn't like it
bcb4f69
@trevorsm7
Add hacked up transform_mutable_iterator
1375e0b
@trevorsm7
Cleanup dead code
995ce9c
@trevorsm7
Actually make mutable iterator work as input iterator
8a95e1b
@trevorsm7
Simplify diff from transform_output_iterator
c88f400
@trevorsm7
Add device implementation of fixed point iterator
b0fe2bf
@trevorsm7
Add test with iterator random access in kernel
a4e266a
@trevorsm7
Merge remote-tracking branch 'rapidsai/branch-0.15' into test-fixed-p…
001f21d 
…oint-iterator
@trevorsm7
Use thrust/transform_input_output_iterator
889baf0
@trevorsm7
Replace rescale with rescaled
5fa4617
@codereport
Add constexpr to numeric::scale_type ctor
ed70d68
@codereport
Cleanup forward declarations
9101b2b
@codereport
Add scale to data_type
13858b2
@codereport
Fixed typo in clamp file
809f00e
@codereport
Merge branch 'branch-0.15' into fp-scale-to-datatype
aecc533
@codereport
Add make_numeric_column
5b7cbec
@codereport
factories_test.cpp remove FixedWidthTypesWithoutFixedPoint
6e2bf32
@codereport codereport added the 2 - In Progress Currently a work in progress label Aug 6, 2020
@codereport codereport requested a review from a team as a code owner August 6, 2020 02:43
@codereport codereport self-assigned this Aug 6, 2020
@GPUtester
Copy link
Collaborator

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@jrhemstad jrhemstad mentioned this pull request Aug 6, 2020
Closed
45 tasks
@harrism harrism mentioned this pull request Oct 11, 2020
Closed
@harrism
Copy link
Member

harrism commented Oct 11, 2020

@codereport if you have no remaining TODOs please mark "ready to merge" when you are ready.

@harrism harrism added 0 - Waiting on Author Waiting for author to respond to review and removed 3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond labels Oct 12, 2020
@codereport
Copy link
Contributor Author

@codereport if you have no remaining TODOs please mark "ready to merge" when you are ready.

I will push a final change tomorrow (not a TODO but a fix). Will be good to merge after that.

@sperlingxx sperlingxx mentioned this pull request Oct 13, 2020
Closed
16 tasks
@codereport codereport added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 0 - Waiting on Author Waiting for author to respond to review labels Oct 13, 2020
@kkraus14 kkraus14 added the libcudf Affects libcudf (C++/CUDA) code. label Oct 13, 2020
@codereport
Copy link
Contributor Author

rerun tests

karthikeyann
karthikeyann requested changes Oct 13, 2020
View reviewed changes
Copy link
Contributor

@karthikeyann karthikeyann left a comment

Choose a reason for hiding this comment

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

Minor changes.
includes and spell check.

cpp/tests/fixed_point/fixed_point_tests.cu Outdated Show resolved Hide resolved
cpp/tests/utilities/column_utilities.cu Outdated Show resolved Hide resolved
cpp/tests/utilities/column_utilities.cu Show resolved Hide resolved
cpp/tests/utilities/column_utilities.cu Outdated Show resolved Hide resolved
cpp/src/jit/type.cpp Outdated Show resolved Hide resolved
cpp/tests/binaryop/binop-integration-test.cpp Outdated Show resolved Hide resolved
cpp/include/cudf_test/column_wrapper.hpp Outdated Show resolved Hide resolved
cpp/include/cudf_test/column_wrapper.hpp Outdated Show resolved Hide resolved
cpp/tests/replace/replace_tests.cpp Show resolved Hide resolved
cpp/src/binaryop/binaryop.cpp Show resolved Hide resolved
@codereport codereport changed the title [REVIEW] fixed_point Column Optimization (store scale in data_type) [REVIEW] fixed_point Column Optimization (store scale in data_type) [skip ci] Oct 13, 2020
@codereport
Copy link
Contributor Author

Note, don't merge until the previous CI finishes running / passes.

@codereport codereport merged commit dd3eb62 into rapidsai:branch-0.16 Oct 13, 2020
@codereport codereport mentioned this pull request Oct 14, 2020
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harrism harrism harrism approved these changes

@davidwendt davidwendt davidwendt approved these changes

@jrhemstad jrhemstad jrhemstad approved these changes

@karthikeyann karthikeyann karthikeyann approved these changes

Assignees

@codereport codereport

Labels
5 - Ready to Merge Testing and reviews complete, ready to merge libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@codereport @GPUtester @harrism @karthikeyann @jrhemstad @davidwendt @kkraus14 @trevorsm7