Remove _getattr_ method in RangeIndex class - #10538
Conversation
skirui-source
added
Python
Codecov Report
@@ Coverage Diff @@
## branch-22.08 #10538 +/- ##
===============================================
Coverage ? 86.33%
===============================================
Files ? 144
Lines ? 22796
Branches ? 0
===============================================
Hits ? 19681
Misses ? 3115
Partials ? 0 Continue to review full report at Codecov.
|
skirui-source
added
non-breaking
skirui-source
added
breaking
skirui-source
added
non-breaking
vyasr
left a comment
vyasr
left a comment
There was a problem hiding this comment.
There's something odd with isna and argsort. On my current branch, I don't get very consistent measurements of the performance of cupy.zeros or cupy.arange. I suspect that it has to do with either underlying memory allocation characteristics or some initialization logic rather than the performance of the API itself, but I'm not 100% sure. I don't quite trust those numbers, though.
The to_arrow measurement, definitely looks like it's problematic. I think that's basically indicating that populating a PyArrow array sequentially on the host is just a lot slower than populating it in parallel on device. My guess is that this is an API where we are probably willing to eat the GPU memory hit of a conversion to an Int64Index rather than the performance hit. Unless @shwina or @galipremsagar disagree (maybe there are Dask impacts that I am unaware of) I would recommend reverting that implementation to just the naive self._as_int64().to_arrow().
|
It looks like there's still a pandas-incompatible benchmark in there. To find these, take your benchmarks and run them like this: That will tell you if any of your benchmarks will fail when run with pandas. See #11122 for more information (hopefully that gets merged soon, then it'll be available on our main docs page). |
In response to this, |
|
Any method that doesn't have a pandas equivalent can't be run with pandas. I added a mark to solve this case. You should decorate those benchmarks with that mark |
|
@vyasr I have resolved the t |
|
@vyasr The updated before/after comparison benchmark results: Benchmarks -- values_host()
Benchmarks -- to_numpy()
Benchmarks -- argsort()
Benchmarks -- nunique()
Benchmarks -- isna()
Benchmarks -- max()
Benchmarks -- min()
|
|
OK, so I have a theory about what's going on with the @shwina @galipremsagar what do you think? |
Yeah. That and even if you carefully see the allocated memory isn't freed up in cupy for other libraries to utilize.. I think we should move forward since this is a known problem. |
|
@vyasr is this PR ready to merge? |
vyasr
left a comment
vyasr
left a comment
There was a problem hiding this comment.
Thanks for all your hard work and all the iterations!
I was going to let Ashwin and/or Prem take a final look just in case they have a strong opinion on the isna benchmark. Let's definitely merge by EOD today though! This PR has been floating around for a long time. If they don't get to it, we can always roll back a change if they feel strongly about it. So unless someone explicitly says otherwise, I would go ahead and merge before you check out for the day.
galipremsagar
left a comment
galipremsagar
left a comment
There was a problem hiding this comment.
Just a placeholder request, will unblock as I finish the review.
galipremsagar
left a comment
galipremsagar
left a comment
There was a problem hiding this comment.
@vyasr Looks like I think it is okay for now to leave the implementation as is, these are the numbers that I was getting:
In [2]: idx = cudf.RangeIndex(0, 1000000)
# This PR (cupy.zeros)
In [3]: %timeit idx.isna()
361 µs ± 78.1 µs per loop (mean ± std. dev. of 7 runs, 1 loop each)
# branch-22.08 (Materializing + isna)
In [3]: %timeit idx.isna()
441 µs ± 14.8 µs per loop (mean ± std. dev. of 7 runs, 1 loop each)Sure they aren't being quite consistent across runs. But we'd be avoiding the materialization step for now, if we see this show up in dask profiles we can think about how to optimize it then.
galipremsagar
added
5 - Ready to Merge
|
@gpucibot merge |
These operators rely on a method that was renamed in #11272 and are also out of sync with the rest of the `RangeIndex` design now that the `__getattr__` overload has been removed (#10538). Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - GALI PREM SAGAR (https://github.com/galipremsagar) URL: #11868
4 participants
This PR helps reduce implicit conversions by minimizing unnecessary
Int64Indexcolumn materialization by the RangeIndex class( PR #9593). Replaces #10388.The following methods have been explicitly implemented for RangeIndex in this PR :
_column, _columns, where, isna, argsort, max, min, nunique, values_host, to_numpy, to_arrow, __array__As demonstrated by the results posted in this comment, on average:
There's an evident performance gain with the new implementations of
values_host, to_numpy, nunique, min and maxisnaandargsortdemonstrate inconsistent measurements of performance perhaps due to memory allocation discrepanciesWhereas
to_arrowandwherestill materialize anInt64Index