-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat: Add strict parameter to pl.concat(how='horizontal') #25452
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
base: main
Are you sure you want to change the base?
Conversation
…d added a corresponding unit test
…dataframes when how=horizontal
… single element and ddof=1 and there are nulls elsewhere in the Series (pola-rs#20077)
…dding a 'strict' keyword argument to concat_df(how='horizontal')
…lementation and added a more robust set of tests on concat
Change-Id: Icd5fb4d566af250b49f25834c931f8d7377e7515
Change-Id: Ie5f9bd91b671ae5f7e31e228f7b5a246d9168ab3
Change-Id: Iddb25d6798666abc0b9374b071b369f3e92cde39
Change-Id: I32f869c406cc0b34386c7c22aceea12495655eb5
Change-Id: I377b4efd92e0fd1a25639bdc723ceaeaea86113e
Change-Id: I9298a909b7f6eaac6893cecdbdd3be3286fd26f1
Change-Id: I74bdd5d39ec41fa5b4fe3371d62cbeac7b34ee62
Change-Id: If50f99a0b18a3a604620a40623a8323cb52d4e78
Change-Id: I5206f843ef735230ef1c2d37f0627a4e74b54b48
|
@orlp I followed the suggestion in #20019 (comment) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25452 +/- ##
==========================================
- Coverage 82.16% 82.16% -0.01%
==========================================
Files 1728 1727 -1
Lines 240097 240195 +98
Branches 3028 3031 +3
==========================================
+ Hits 197277 197353 +76
- Misses 42040 42061 +21
- Partials 780 781 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
| let out_df = concat_df_horizontal(&out, false)?; | ||
| let strict_concat = matches!(self.extend_behavior, ExtendBehavior::Raise); | ||
| let out_df = concat_df_horizontal(&out, false, strict_concat)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why that is true.
If we have ExtendBehavior::FillNulls, couldn't we have one input head that has data with a different height than others, in which we case we would want to concat without strict?
I don't see where that happens in the code either
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are in the branch all input heads are broadcast inputs, meaning they have length 1.
| Only relevant for LazyFrames. This determines if the concatenated | ||
| lazy computations may be executed in parallel. | ||
| strict | ||
| When how=`horizontal`, require all DataFrames to be the same height |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing period. Can you also emphasize that this is only to disable broadcasting? Different heights are never allowed anyway, only for broadcasting:
When how=`horizontal`, require all DataFrames to be the same height, raising an error instead of broadcasting unit height DataFrames.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are different heights never allowed?
In this example I have two non-unit dataframes being concatenated together: #25263 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, they're never allowed.
EDIT: my bad, you're right. Please disregard this. I'm too deep in the implementation of things where this is never allowed that I forgot this is the API by which users can request the ExtendWithNulls behavior.
Change-Id: I0934aa1fdabe12b6a333f0c97ec0958d0a545175
|
Can you address the final comment and then do a rebase on |
PR that closes #19133
Adopted from #20019