-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
fix(rust): Prevent panic on sample_n with replacement from empty df #10731
Conversation
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.
Thanks for the fix. I have left a few minor comments.
@@ -9,6 +9,9 @@ use crate::random::get_global_random_u64; | |||
use crate::utils::{CustomIterTools, NoNull}; | |||
|
|||
fn create_rand_index_with_replacement(n: usize, len: usize, seed: Option<u64>) -> IdxCa { | |||
if len == 0 { | |||
return NoNull::<IdxCa>::from_iter(std::iter::empty()).into_inner(); |
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.
Can you use IdxCa::new_empty
I added the test to py-polars and confirmed that without the fix we hit a panic
I do not think there is a way to use |
Nice CI and make setup! This was pretty streamlined and intuitive for making contributions for the first time to this project! |
Nice! Honours to @stinodego on this one. ;) And thanks a lot for the fix! |
Sampling n from an empty dataframe panics if with_replacement=True. A test case is added documenting the behavior of sample_n and sample_frac for with_replacement and without.
I am not really sure why ensure_shape is used without symmetry, but it appears intentional.