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

[WIP] Allocate validity masks on output arrays of groupbys #246

Closed
wants to merge 11 commits into from

Conversation

kkraus14
Copy link
Collaborator

No description provided.

@kkraus14
Copy link
Collaborator Author

This looks to break the current groupby sort based implementation, I guess it doesn't set the valid bits as it fills the output columns?

@scopatz
Copy link
Contributor

scopatz commented Sep 20, 2018

Yeah, it looks like it is reducing the array to length-0 on output.

@kkraus14 kkraus14 changed the title [REVIEW] Allocate validity masks on output arrays of groupbys [WIP] Allocate validity masks on output arrays of groupbys Sep 20, 2018
@dantegd dantegd added the 2 - In Progress Currently a work in progress label Sep 21, 2018
@kkraus14
Copy link
Collaborator Author

Depends on #261

@mike-wendt
Copy link
Contributor

@kkraus14 what is the status now that #261 is merged? I know this is blocking rapidsai/libgdf#140

@kkraus14
Copy link
Collaborator Author

rerun tests

1 similar comment
@harrism
Copy link
Member

harrism commented Oct 25, 2018

rerun tests

@harrism
Copy link
Member

harrism commented Oct 25, 2018

@kkraus14 now that the join syntax error is gone these are failing with GDF_VALIDITY_MISSING, can you take a look?

@@ -300,8 +301,9 @@ def gpu_mask_from_devary(ary, bits):

def mask_from_devary(ary):
bits = make_mask(len(ary))
gpu_fill_value.forall(bits.size)(bits, 0)
gpu_mask_from_devary.forall(bits.size)(ary, bits)
if bits.size > 0:

Choose a reason for hiding this comment

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

is this function ensuring that the bitmasks are 64 byte padded as in the arrow spec? Or if not that at least 64 bit aligned since there are places in the code that assume this so that we can read 64 values at a time from memory?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, currently it seems like it's not padded and uses 1 byte as underlying type. @jrhemstad these are functions we'd want to move to the bit utils anyway, correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

@felipeblazing we're working on updating the gdf_valid_type and associated bit utility functions. I believe @BradReesWork can comment on that effort.

Choose a reason for hiding this comment

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

ok great!

@harrism harrism added Python Affects Python cuDF API. 0 - Waiting on Author Waiting for author to respond to review and removed 2 - In Progress Currently a work in progress labels Nov 25, 2018
@kkraus14 kkraus14 added 2 - In Progress Currently a work in progress and removed 0 - Waiting on Author Waiting for author to respond to review labels Nov 28, 2018
@kkraus14
Copy link
Collaborator Author

Looks like a lot of the test failures were related to the concat bug fixed in #427, hopefully there's a more manageable number now.

@harrism harrism changed the base branch from master to branch-0.5 December 10, 2018 04:23
@harrism
Copy link
Member

harrism commented Jan 8, 2019

@kkraus14 we will not be getting null support for groupby into v0.5. I believe this PR is solely needed for null support on groupby, so can we also remove this from v0.5?

@kkraus14
Copy link
Collaborator Author

kkraus14 commented Mar 8, 2019

Closing this as it's outdated.

@kkraus14 kkraus14 closed this Mar 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 - In Progress Currently a work in progress Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants