-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
.list.index_of_in() architectural review PR #20733
base: main
Are you sure you want to change the base?
Conversation
It's also possible the casting logic in Polars will need to be extended; |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20733 +/- ##
==========================================
- Coverage 79.75% 79.31% -0.45%
==========================================
Files 1561 1580 +19
Lines 221785 224397 +2612
Branches 2530 2573 +43
==========================================
+ Hits 176885 177970 +1085
- Misses 44318 45839 +1521
- Partials 582 588 +6 ☔ View full report in Codecov by Sentry. |
I am not sure we want this. The compilation bloat of this seems very high to me and we've got |
So the code in the PR doesn't have much in the way of generics or templates, at least at the moment, so it doesn't seem like it adds much to compilation? Or is it that you're worried about the cumulative costs of lots of small features adding up? |
AFAICT df = pl.DataFrame({"lists": [[3, 1], [2, 4], [5, 3, 1]], "values": [1, 2, 6]})
result = df.select(pl.col("lists").list.index_of_in(pl.col("values"))) because you can't use @orlp had a pure-Python suggestion in the issue, but it was certainly beyond my admittedly very basic Polars query-writing skills: result2 = df.with_row_index().select(
pl.col("lists").explode().index_of(pl.col("values").first()).over("index")
)
print(result2) If this technique is acceptable another option is a pure Python |
Actually, not clear you can actually apply that technique with index inside expression function, so that might not be a viable approach. |
@ritchie46 could you expand about your concerns re compilation? See above. Thank you! |
I think this is a good feature, I would also like to see You can use df.select("lists","values",index_of = pl.col("lists").list.concat(pl.col("values")).list.eval(pl.element().slice(0,pl.element().len()-1).index_of(pl.element().last())).list.first()) In a test with this df df=(
pl.DataFrame({"a":np.random.uniform(0,100,100_000),
"lists":np.random.uniform(0,10,100_000)})
.with_columns(pl.all().cast(pl.Int64))
.group_by("a")
.agg('lists')
.select("lists", values=pl.lit(pl.Series(np.random.uniform(0,10,100)).cast(pl.Int64)))
) I get 4.55ms for my method and 8ms for the explode/over approach so having something native that does even better than a .list.eval would be cool. |
Looks like that method might be implementable as a method on the Python |
So, trying the Python version using
Conclusion: this feature seems doable with just Python, but at the cost of a massive 10× slowdown. |
This PR implements a sketch of
.list.index_of_in()
as described in #20626The questions I would definitely like feedback on at the moment are:
amortized_iter()
the right approach?needles.iter()
the right approach?