fix: Do not allow inferring (-1) the dimension on any Expr.reshape dimension except the first
#24591
Conversation
| } | ||
|
|
||
| let num_infers = dims.iter().filter(|d| matches!(d, ReshapeDimension::Infer)).count(); | ||
| let infers = dims.iter().enumerate().filter(|(_, d)| matches!(d, ReshapeDimension::Infer)).collect::<Vec<_>>(); |
There was a problem hiding this comment.
By definition, if you cannot infer anything beyond the first dimension, you cannot infer more than 1 dimension. I think you can do an .any() on dims[1..].
|
Addressed the nit, and also updated the docstring for |
| } | ||
|
|
||
| polars_ensure!(num_infers <= 1, InvalidOperation: "can only specify one inferred dimension"); | ||
| let num_infers = dims |
There was a problem hiding this comment.
I think this can now be usize::from(matches!(dims[0], ReshapeDimension::Infer)). But, in general, if we can just have a boolean here now saying infers_dimension as there is now only 1 dimension to infer.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24591 +/- ##
==========================================
+ Coverage 81.79% 81.81% +0.01%
==========================================
Files 1686 1686
Lines 229972 229985 +13
Branches 2974 2974
==========================================
+ Hits 188107 188155 +48
+ Misses 41112 41077 -35
Partials 753 753 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
It still seems possible to create an expr with -1 in a dimension other than the first, so I'm wondering if it's worth checking the dimension in the reshape function. |
Related issue: #24536
Sibling PR: #24590
Cc @coastalwhite