Closed bobot closed 3 months ago
Also it works if the row with a=1 (so n=1) is first.
I faced with the same problem. Polars version is 0.20.25
I'm taking a look at this.
The implementation devolves to lst_sample_n
(in polars_core::chunked_array::list::namespace
). Here's the relevant snippet:
ca.try_zip_and_apply_amortized(n, |opt_s, opt_n| {
match (opt_s, opt_n) {
(Some(s), Some(n)) => s
.as_ref()
.sample_n(n as usize, with_replacement, shuffle, seed)
.map(Some),
_ => Ok(None),
}})
Here's the sample_n()
function, from polars_core::chunked_array::random
:
impl Series {
pub fn sample_n(
&self,
n: usize,
with_replacement: bool,
shuffle: bool,
seed: Option<u64>,
) -> PolarsResult<Self> {
ensure_shape(n, self.len(), with_replacement)?;
if n == 0 {
return Ok(self.clear());
}
let len = self.len();
match with_replacement {
true => {
let idx = create_rand_index_with_replacement(n, len, seed);
debug_assert_eq!(len, self.len());
// SAFETY: we know that we never go out of bounds.
unsafe { Ok(self.take_unchecked(&idx)) }
},
false => {
let idx = create_rand_index_no_replacement(n, len, seed, shuffle);
debug_assert_eq!(len, self.len());
// SAFETY: we know that we never go out of bounds.
unsafe { Ok(self.take_unchecked(&idx)) }
},
}
}
Changing self.clear()
to Series::new_empty(self.name(), self.dtype())
seems to solve the problem!
Given clear()
takes &self
, you wouldn't expect it to cause side-effects. Except that try_zip_and_apply_amortized()
uses UnstableSeries
, so my current suspicion is that somewhere in there the invariants are wrong or not being applied. Which means this is plausibly a bigger problem, certainly this change doesn't seem like it should help. And yet...
@reswqa I think maybe you wrote the original code? Do you have any insight?
Documentation for UnstableSeries
says:
/// A wrapper type that should make it a bit more clear that we should not clone Series
But the API makes it trivial to clone() (my_unstable_series.as_ref().clone()
), and in fact that is was Series::clear() was doing in this particular edge case.
Here is a demonstration of what I believe is unsound behavior of the UnstableSeries
API:
fn undefined_behavior() {
let mut series = Series::new("a", [1, 2]);
let mut_ref = &mut series;
let addr1 = std::ptr::addr_of!(*mut_ref);
let mut unstable = UnstableSeries::new(mut_ref);
// This won't compile:
// let non_mut_ref = series.clone();
// But we can now have both a mutable reference and a non-mutable
// reference at the same time a different way, which is completely
// unsound:
let non_mut_ref = unstable.as_ref();
let addr2 = std::ptr::addr_of!(*non_mut_ref);
assert_eq!(addr1 as usize, addr2 as usize);
let cloned = non_mut_ref.clone();
// And now we can mutate cloned, at a distance:
let series2 = Series::new("b", [3, 4]);
unstable.swap(&mut series2.array_ref(0).clone());
assert_eq!(cloned.sum::<usize>().unwrap(), 7);
}
Checks
Reproducible example
Log output
Issue description
Using Expr.list.sample with n=0 in some rows but not all can raise an unexpected error.
Expected behavior
It works for k=1 p=0, and seems to work with p>0. So the sampling with some rows equal to 0 seems the heart of the problem.
Installed versions