Skip to content

Use Index._column in favor of Index._values - #15494

Closed
mroeschke wants to merge 1 commit into
NVIDIA:branch-24.06from
mroeschke:ref/index/_values
Closed

Use Index._column in favor of Index._values#15494
mroeschke wants to merge 1 commit into
NVIDIA:branch-24.06from
mroeschke:ref/index/_values

Conversation

@mroeschke

Copy link
Copy Markdown
Contributor

Description

The base definition is

    @property  # type: ignore
    @_cudf_nvtx_annotate
    def _values(self):
        return self._column

So I think it's just more straightforward to use self._columns

Checklist

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

@mroeschke mroeschke added Python Affects Python cuDF API. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Apr 9, 2024
@mroeschke
mroeschke requested a review from a team as a code owner April 9, 2024 23:47
@mroeschke
mroeschke requested review from galipremsagar and vyasr April 9, 2024 23:47
@mroeschke mroeschke changed the title Use Index._columns in favor of Index._values Use Index._column in favor of Index._values Apr 9, 2024
@cached_property # type: ignore
@_cudf_nvtx_annotate
def _values(self):
def _column(self):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This change is somewhat conceptually problematic. A RangeIndex isn't backed by a column. My hope was to be able to rewrite algorithms in cudf to never rely on this kind of method at all, so as to be agnostic to the type of index and not accidentally materialize GPU data (see #9593). Should we prioritize those changes rather than making a cosmetic improvement like this one that is likely to only increase the frequency with which we accidentally allocate device memory?

@mroeschke mroeschke Apr 17, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

An effort to remove BaseIndex._values entirely as described in #9593 seems like a more worthy undertaking.

Curious how you envisioned intentionally materializing a RangeIndex to a column? as_column?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think a good starting point would be removing the ability to materialize a RangeIndex (i.e. removing those code paths from as_column and methods like RangeIndex._values altogether) and then seeing what breaks. In an ideal world we would implement versions of our code that avoid materialization altogether.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK some initial operations that would break:

  1. .reset_index where the index is a RangeIndex i.e. the RangeIndex needs to materialized to a new column
  2. RangeIndex.equals e.g. an Index[int64] can equals a RangeIndex. (I suppose materialization can be avoided by checking if an Index can be "compressed" to a range)
  3. RangeIndex.astype to a non int64 type

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

  1. My gut reaction is that this seems wrong. If the index is a RangeIndex, then the output of reset_index should also be a RangeIndex. pandas bears this out
>>> df = pd.DataFrame({'a': [1, 2, 3]})
>>> df.index
RangeIndex(start=0, stop=3, step=1)
>>> df.reset_index().index
RangeIndex(start=0, stop=3, step=1)

where does it break for us?
2. I think figuring out whether an integer index can be "compressed" to a range is going to be more work to implement than it's worth. To do that without allocating any intermediate memory for the diffs would require a slightly nontrivial algorithm to do something like compute the steps between every pair of elements, then do some synchronizations of a local variable a la a reduction. A simpler approach that would still require some GPU code but would be conceptually very straightforward would be to implement a libcudf function that allows you to check whether a column is equal to a range by specifying the range as a smart iterator, e.g. a counting iterator from start to stop with the step. That would be almost trivial to implement in C++. I don't think it's worth doing this right now since there would be some additional work to make that available in Python and then branch accordingly in Column.equals instead of calling a binop, but maybe we open an issue with the suggestion and leave it as a good first issue for libcudf (unless you're interested in tackling it yourself!).
3. We obviously can't avoid materializing in this case, but can we change how we handle this? For example, here's a one-liner that gets us what we need and would allow us to never need to use RangeIndex._values:

>>> df = pd.DataFrame({'a': [1, 2, 3]})
>>> df.index
RangeIndex(start=0, stop=3, step=1)
>>> cp.asarray(df.index, dtype=np.int32)
array([0, 1, 2], dtype=int32)

rapids-bot Bot pushed a commit that referenced this pull request May 7, 2024
xref #15494

If the attributes are exposed on the top level object e.g. `Index.dtype` it should be sufficient to just access the attributes there instead of reaching for the underlying object

Authors:
  - Matthew Roeschke (https://github.com/mroeschke)

Approvers:
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #15624
rapids-bot Bot pushed a commit that referenced this pull request May 21, 2024
xref #15494

* For `Index.str`, check the `dtype` instead of the underlying column type (which would materialize RangeIndex)
* For `set_index`, don't immediately convert passed objects to column until necessary
* For `_make_operands_and_index_for_binop`, don't create pandas object more than once

Authors:
  - Matthew Roeschke (https://github.com/mroeschke)
  - GALI PREM SAGAR (https://github.com/galipremsagar)

Approvers:
  - GALI PREM SAGAR (https://github.com/galipremsagar)

URL: #15763
@vyasr

vyasr commented May 22, 2024

Copy link
Copy Markdown
Collaborator

@mroeschke should we close this PR? Is it useful for tracking purposes? IIRC based on the main discussion above we're going in a different direction altogether to try and reduce materializations.

@mroeschke

Copy link
Copy Markdown
Contributor Author

It's useful for tracking but it doesn't need to remain open so I'll close this

@mroeschke mroeschke closed this May 22, 2024
@mroeschke
mroeschke deleted the ref/index/_values branch May 22, 2024 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Affects Python cuDF API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants