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

Use CuPy array in pip_bitmap_column_to_binary_array #1418

Merged
merged 5 commits into from
Jul 31, 2024

Conversation

isVoid
Copy link
Contributor

@isVoid isVoid commented Jul 30, 2024

Description

The performance regression in #1413 is due to numba's DeviceNDArray
is slow in slicing. Recent cudf's DataFrame construction has simplified the construction and delegated construction
to similar logic that handles cuda_array_interface. Since the construction involves slicing the array, we need
this operation to be fast. In that sense, we should cast the use of DeviceNDArray to cupy array to support fast
slicing.

closes #1413

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@isVoid isVoid requested a review from a team as a code owner July 30, 2024 14:09
@isVoid isVoid requested review from trxcllnt and thomcom July 30, 2024 14:09
@isVoid isVoid changed the title Cast result of pip_bitmap_column_to_binary_array to cupy array Cast result of pip_bitmap_column_to_binary_array to cupy array Jul 30, 2024
@github-actions github-actions bot added the Python Related to Python code label Jul 30, 2024
@trxcllnt
Copy link
Contributor

Looks like the test needs to be updated as well:

    def test_pip_bitmap_column_to_binary_array():
        col = cudf.Series([0b00000000, 0b00001101, 0b00000011, 0b00001001])._column
        got = pip_bitmap_column_to_binary_array(col, width=4)
        expected = np.array(
            [[0, 0, 0, 0], [1, 1, 0, 1], [0, 0, 1, 1], [1, 0, 0, 1]], dtype="int8"
        )
>       np.testing.assert_array_equal(got.copy_to_host(), expected)
E       AttributeError: 'ndarray' object has no attribute 'copy_to_host'

@@ -31,7 +32,7 @@ def apply_binarize(in_col, width):
if out.size > 0:
out[:] = 0
binarize.forall(out.size)(in_col, out, width)
return out
return cp.asarray(out)
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is 2D, you could use asfortranarray so when sliced into 1D columns the memory is contiguous. (I'll also ensure this on the cudf side)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

asfortranarray will reorder the data to F-contiguous with new allocation and increases peak memory usage. It would be nice if we don't have to assume memory order for return values? Cupy support fast indexing for any memory order anyway.

Copy link
Member

Choose a reason for hiding this comment

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

If we would like to change the order of the array, this could be done by passing the order flag to reshape

That said, given this wasn't done before with the DeviceNDArray (and may involve other subtle changes in associated code), maybe this should be converted to a new issue/PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe this should be converted to a new issue/PR?

Yes - again, unless explicitly required by downstream libraries, I would rather cuspatial not having to dictate a memory order here but just stick to the defaults. Switching to cupy gives the flexibility not having to be explicit on memory orders, and cudf is adding back support to different memory order arrays. This relieves the responsibility from cuspatial.

@harrism harrism added bug Something isn't working non-breaking Non-breaking change labels Jul 30, 2024
@jakirkham jakirkham changed the title Cast result of pip_bitmap_column_to_binary_array to cupy array Use CuPy array in pip_bitmap_column_to_binary_array Jul 31, 2024
@jakirkham
Copy link
Member

Have retitled the PR to reflect the current state. Hope that is ok 🙂

@harrism
Copy link
Member

harrism commented Jul 31, 2024

/merge

@rapids-bot rapids-bot bot merged commit fe3b0c9 into rapidsai:branch-24.08 Jul 31, 2024
69 checks passed
@harrism harrism mentioned this pull request Jul 31, 2024
3 tasks
@harrism harrism mentioned this pull request Jul 31, 2024
3 tasks
rapids-bot bot pushed a commit that referenced this pull request Aug 1, 2024
#1407 skipped the taxi dropoffs notebook due to performance regression fixed in #1418, so this PR re-enables the notebook in CI by removing it from SKIPNBS in ci/test_noteboks.sh.

Authors:
  - Mark Harris (https://github.com/harrism)

Approvers:
  - https://github.com/jakirkham
  - Michael Wang (https://github.com/isVoid)
  - James Lamb (https://github.com/jameslamb)

URL: #1422
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-breaking Non-breaking change Python Related to Python code
Projects
Status: Review
Development

Successfully merging this pull request may close these issues.

[BUG]: possible performance regression in points_in_polygon()
5 participants