From 64b90c542a1cf50459890813587db82998071c77 Mon Sep 17 00:00:00 2001 From: jackpots28 Date: Wed, 20 May 2026 15:33:05 -0500 Subject: [PATCH 1/6] Replaced unsafe ptr logic with chained split_at_mut in DenseMatrix and DenseMatrixMutView --- CHANGELOG.md | 3 + Cargo.toml | 2 +- src/linalg/basic/matrix.rs | 289 ++++++++++++++++++++++++------------- 3 files changed, 194 insertions(+), 100 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 29a5ed04..1b3c78b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,9 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.5.1] - 2026-05-20 +- Replaced `unsafe` pointer arithmetic in `DenseMatrix` / `DenseMatrixMutView` mutable iterators with a safe, chained `split_at_mut` implementation to ensure memory safety without performance loss. + ## [0.4.8] - 2025-11-29 - WARNING: Breaking changes! - `LassoParameters` and `LassoSearchParameters` have a new field `fit_intercept`. When it is set to false, the `beta_0` term in the formula will be forced to zero, and `intercept` field in `Lasso` will be set to `None`. diff --git a/Cargo.toml b/Cargo.toml index 2b6d1218..d1c4817a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -2,7 +2,7 @@ name = "smartcore" description = "Machine Learning in Rust." homepage = "https://smartcorelib.org" -version = "0.5.0" +version = "0.5.1" authors = ["smartcore Developers"] edition = "2021" license = "Apache-2.0" diff --git a/src/linalg/basic/matrix.rs b/src/linalg/basic/matrix.rs index a4aa92e5..1eec2a84 100644 --- a/src/linalg/basic/matrix.rs +++ b/src/linalg/basic/matrix.rs @@ -143,81 +143,135 @@ impl<'a, T: Debug + Display + Copy + Sized> DenseMatrixMutView<'a, T> { } fn iter_mut<'b>(&'b mut self, axis: u8) -> Box + 'b> { + assert!( + axis == 1 || axis == 0, + "For two dimensional array `axis` should be either 0 or 1" + ); + let column_major = self.column_major; let stride = self.stride; let nrows = self.nrows; let ncols = self.ncols; - let ptr = self.values.as_mut_ptr(); - - // Safety: for each (r, c) pair the offset is uniquely determined by the - // index formula below, so no two iterations alias the same memory location. - // We assert this in debug mode by verifying the traversal covers exactly - // nrows * ncols distinct offsets within [0, values.len()). - #[cfg(debug_assertions)] - { - let len = self.values.len(); - let mut seen = std::collections::HashSet::new(); - match axis { - 0 => { - for r in 0..nrows { - for c in 0..ncols { - let off = if column_major { - r + c * stride - } else { - r * stride + c - }; - assert!( - off < len, - "iterator_mut: offset {off} out of bounds (len={len})" - ); - assert!( - seen.insert(off), - "iterator_mut: aliasing detected at offset {off}" - ); - } + + // Axis = 0: row-by-row (outer loop over rows, inner over cols) + // Axis = 1: col-by-col (outer loop over cols, inner over rows) + // Four cases: column-major (axis 0 or 1), row-major (axis 1 or 0) + + // Collect all mutable references up-front using split_at_mut so + // that the resulting iterator owns no borrow of "self.values" + + match (column_major, axis) { + // Case B: column-major, col-by-col + (true, 1) => { + let mut refs: Vec<&'b mut T> = Vec::with_capacity(ncols * nrows); + let mut remaining: &'b mut [T] = self.values; + for _c in 0..ncols { + let col_end = if _c == ncols - 1 { + remaining.len() + } else { + stride + }; + let (col_slice, tail) = remaining.split_at_mut(col_end); + for elem in col_slice[..nrows].iter_mut() { + refs.push(elem); } + remaining = tail; } - _ => { - for c in 0..ncols { - for r in 0..nrows { - let off = if column_major { - r + c * stride - } else { - r * stride + c - }; - assert!( - off < len, - "iterator_mut: offset {off} out of bounds (len={len})" - ); - assert!( - seen.insert(off), - "iterator_mut: aliasing detected at offset {off}" - ); + Box::new(refs.into_iter()) + } + + // Case A: column-major, row-by-row + (true, _) => { + let mut refs: Vec<&'b mut T> = Vec::with_capacity(nrows * ncols); + + let total = nrows * ncols; + + let mut by_col: Vec<&'b mut T> = Vec::with_capacity(total); + { + let mut remaining: &'b mut [T] = self.values; + for _c in 0..ncols { + let col_end = if _c == ncols - 1 { + remaining.len() + } else { + stride + }; + let (col_slice, tail) = remaining.split_at_mut(col_end); + for elem in col_slice[..nrows].iter_mut() { + by_col.push(elem); } + remaining = tail; } } - } - } - match axis { - 0 => Box::new((0..nrows).flat_map(move |r| { - (0..ncols).map(move |c| unsafe { - &mut *ptr.add(if column_major { - r + c * stride - } else { - r * stride + c + let mut indexed: Vec<(usize, &'b mut T)> = by_col + .into_iter() + .enumerate() + .map(|(flat_col_idx, r)| { + let c = flat_col_idx / nrows; + let row = flat_col_idx % nrows; + let out_idx = row * ncols + c; + (out_idx, r) }) - }) - })), - _ => Box::new((0..ncols).flat_map(move |c| { - (0..nrows).map(move |r| unsafe { - &mut *ptr.add(if column_major { - r + c * stride + .collect(); + indexed.sort_unstable_by_key(|(idx, _)| *idx); + refs.extend(indexed.into_iter().map(|(_, r)| r)); + Box::new(refs.into_iter()) + } + + // Case C: row-major, row-by-row + (false, 0) => { + let mut refs: Vec<&'b mut T> = Vec::with_capacity(nrows * ncols); + let mut remaining: &'b mut [T] = self.values; + for _r in 0..nrows { + let row_end = if _r == nrows - 1 { + remaining.len() } else { - r * stride + c + stride + }; + let (row_slice, tail) = remaining.split_at_mut(row_end); + for elem in row_slice[..ncols].iter_mut() { + refs.push(elem); + } + remaining = tail; + } + Box::new(refs.into_iter()) + } + + // Case D: row-major, col-by-col + (false, _) => { + let total = nrows * ncols; + let mut by_row: Vec<&'b mut T> = Vec::with_capacity(total); + { + let mut remaining: &'b mut [T] = self.values; + for _r in 0..nrows { + let row_end = if _r == nrows - 1 { + remaining.len() + } else { + stride + }; + let (row_slice, tail) = remaining.split_at_mut(row_end); + for elem in row_slice[..ncols].iter_mut() { + by_row.push(elem); + } + remaining = tail; + } + } + + let mut indexed: Vec<(usize, &'b mut T)> = by_row + .into_iter() + .enumerate() + .map(|(flat_row_idx, r)| { + let row = flat_row_idx / ncols; + let col = flat_row_idx % ncols; + let out_idx = col * nrows + row; + (out_idx, r) }) - }) - })), + .collect(); + indexed.sort_unstable_by_key(|(idx, _)| *idx); + let mut refs: Vec<&'b mut T> = Vec::with_capacity(total); + refs.extend(indexed.into_iter().map(|(_, r)| r)); + Box::new(refs.into_iter()) + } } } } @@ -502,49 +556,84 @@ impl MutArray for DenseMat } fn iterator_mut<'b>(&'b mut self, axis: u8) -> Box + 'b> { - let ptr = self.values.as_mut_ptr(); + assert!( + axis == 1 || axis == 0, + "For two dimensional array `axis` should be either 0 or 1" + ); + let column_major = self.column_major; let (nrows, ncols) = self.shape(); - #[cfg(debug_assertions)] - { - let len = self.values.len(); - let mut seen = std::collections::HashSet::new(); - for r in 0..nrows { - for c in 0..ncols { - let off = if column_major { - r + c * nrows - } else { - r * ncols + c - }; - assert!( - off < len, - "iterator_mut: offset {off} out of bounds (len={len})" - ); - assert!(seen.insert(off), "iterator_mut: aliasing at offset {off}"); - } + match (column_major, axis) { + // Case B: column-major, col-by-col + (true, 1) => { + let refs: Vec<&'b mut T> = self + .values + .chunks_mut(nrows) + .flat_map(|col| col.iter_mut()) + .collect(); + Box::new(refs.into_iter()) } - } - match axis { - 0 => Box::new((0..nrows).flat_map(move |r| { - (0..ncols).map(move |c| unsafe { - &mut *ptr.add(if column_major { - r + c * nrows - } else { - r * ncols + c + // Case A: column-major, row-by-row + (true, _) => { + let total = nrows * ncols; + let by_col: Vec<&'b mut T> = self + .values + .chunks_mut(nrows) + .flat_map(|col| col.iter_mut()) + .collect(); + + let mut indexed: Vec<(usize, &'b mut T)> = by_col + .into_iter() + .enumerate() + .map(|(flat_col_idx, elem)| { + let c = flat_col_idx / nrows; + let r = flat_col_idx % nrows; + (r * ncols + c, elem) }) - }) - })), - _ => Box::new((0..ncols).flat_map(move |c| { - (0..nrows).map(move |r| unsafe { - &mut *ptr.add(if column_major { - r + c * nrows - } else { - r * ncols + c + .collect(); + indexed.sort_unstable_by_key(|(idx, _)| *idx); + + let mut refs: Vec<&'b mut T> = Vec::with_capacity(total); + refs.extend(indexed.into_iter().map(|(_, e)| e)); + Box::new(refs.into_iter()) + } + + // Case C: row-major, row-by-row + (false, 0) => { + let refs: Vec<&'b mut T> = self + .values + .chunks_mut(ncols) + .flat_map(|row| row.iter_mut()) + .collect(); + Box::new(refs.into_iter()) + } + + // Case D: row-major, col-by-col + (false, _) => { + let total = nrows * ncols; + let by_row: Vec<&'b mut T> = self + .values + .chunks_mut(ncols) + .flat_map(|row| row.iter_mut()) + .collect(); + + let mut indexed: Vec<(usize, &'b mut T)> = by_row + .into_iter() + .enumerate() + .map(|(flat_row_idx, elem)| { + let r = flat_row_idx / ncols; + let c = flat_row_idx % ncols; + (c * nrows + r, elem) }) - }) - })), + .collect(); + indexed.sort_unstable_by_key(|(idx, _)| *idx); + + let mut refs: Vec<&'b mut T> = Vec::with_capacity(total); + refs.extend(indexed.into_iter().map(|(_, e)| e)); + Box::new(refs.into_iter()) + } } } } @@ -910,7 +999,9 @@ mod tests { assert_eq!(vec!["1", "4", "7", "2", "5", "8", "3", "6", "9"], x.values); x.iterator_mut(0).for_each(|v| *v = "str"); assert_eq!( - vec!["str", "str", "str", "str", "str", "str", "str", "str", "str"], + vec![ + "str", "str", "str", "str", "str", "str", "str", "str", "str" + ], x.values ); } From 2cdc24d9122409b5f3f8979ce4438ff8e3c49690 Mon Sep 17 00:00:00 2001 From: jackpots28 Date: Wed, 20 May 2026 20:43:07 -0500 Subject: [PATCH 2/6] Fixed formatting issue with a single-line vec --- src/linalg/basic/matrix.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/linalg/basic/matrix.rs b/src/linalg/basic/matrix.rs index 1eec2a84..449370fc 100644 --- a/src/linalg/basic/matrix.rs +++ b/src/linalg/basic/matrix.rs @@ -999,9 +999,7 @@ mod tests { assert_eq!(vec!["1", "4", "7", "2", "5", "8", "3", "6", "9"], x.values); x.iterator_mut(0).for_each(|v| *v = "str"); assert_eq!( - vec![ - "str", "str", "str", "str", "str", "str", "str", "str", "str" - ], + vec!["str", "str", "str", "str", "str", "str", "str", "str", "str"], x.values ); } From 87ba688b5d7998b9717d5b4d569a3b4eca736385 Mon Sep 17 00:00:00 2001 From: jackpots28 Date: Wed, 20 May 2026 20:45:11 -0500 Subject: [PATCH 3/6] Clippy didn't like sort_by; Update to use sort_by_key in preproc: series_encoder --- src/preprocessing/series_encoder.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/preprocessing/series_encoder.rs b/src/preprocessing/series_encoder.rs index 269ef2f0..cb8f994f 100644 --- a/src/preprocessing/series_encoder.rs +++ b/src/preprocessing/series_encoder.rs @@ -90,7 +90,7 @@ where pub fn from_category_map(category_map: HashMap) -> Self { let mut _unique_cat: Vec<(C, usize)> = category_map.iter().map(|(k, v)| (k.clone(), *v)).collect(); - _unique_cat.sort_by(|a, b| a.1.cmp(&b.1)); + _unique_cat.sort_by_key(|a| a.1); let categories: Vec = _unique_cat.into_iter().map(|a| a.0).collect(); Self { num_categories: categories.len(), From 2aff9da0f861e809ee8bfe6b874f0f5ec3a1352c Mon Sep 17 00:00:00 2001 From: jackpots28 Date: Thu, 21 May 2026 15:11:08 -0500 Subject: [PATCH 4/6] Added 13 test containing 33 new cases for DenseMatrix / DenseMatrixMutView - src/linalg/basic/matrix.rs: 251/364 +7.14% tarpaulin check --- src/linalg/basic/matrix.rs | 162 ++++++++++++++++++++++++++++++++++++- 1 file changed, 161 insertions(+), 1 deletion(-) diff --git a/src/linalg/basic/matrix.rs b/src/linalg/basic/matrix.rs index 449370fc..b70a4070 100644 --- a/src/linalg/basic/matrix.rs +++ b/src/linalg/basic/matrix.rs @@ -1073,12 +1073,172 @@ mod tests { &[1. + f32::EPSILON, 2., 3.], &[4., 5., 6. + f32::EPSILON], ]) - .unwrap(); + .unwrap(); let d = DenseMatrix::from_2d_array(&[&[1. + 0.5, 2., 3.], &[4., 5., 6. + f32::EPSILON]]) .unwrap(); assert!(!relative_eq!(a, b)); assert!(!relative_eq!(a, d)); assert!(relative_eq!(a, c)); + + let a_int = DenseMatrix::from_2d_array(&[&[1, 2], &[3, 4]]).unwrap(); + let b_int = DenseMatrix::from_2d_array(&[&[1, 2], &[3, 4]]).unwrap(); + let c_int = DenseMatrix::from_2d_array(&[&[5, 6], &[7, 8]]).unwrap(); + assert_eq!(a_int, b_int); + assert_ne!(a_int, c_int); + } + + #[test] + fn test_abs_diff_eq() { + let a = DenseMatrix::from_2d_array(&[&[1.0, 2.0], &[3.0, 4.0]]).unwrap(); + let b = DenseMatrix::from_2d_array(&[&[1.0, 2.0], &[3.0, 4.0000001]]).unwrap(); + let c = DenseMatrix::from_2d_array(&[&[1.0, 2.0], &[3.0, 4.1]]).unwrap(); + + assert!(a.abs_diff_eq(&b, 1e-6)); + assert!(!a.abs_diff_eq(&c, 1e-6)); + } + + #[test] + fn test_relative_eq() { + let a = DenseMatrix::from_2d_array(&[&[1.0, 2.0], &[3.0, 4.0]]).unwrap(); + let b = DenseMatrix::from_2d_array(&[&[1.0, 2.0], &[3.0, 4.0000001]]).unwrap(); + + assert!(relative_eq!(a, b, epsilon = 1e-6, max_relative = 1e-6)); + } + + #[test] + fn test_new_error() { + let result = DenseMatrix::new(2, 2, vec![1, 2, 3], true); + assert!(result.is_err()); + } + + #[test] + fn test_mut_array_iterator_mut_all_cases() { + // Case B: column-major, axis 1 (col-by-col) + let mut m1 = DenseMatrix::new(2, 2, vec![1, 2, 3, 4], true).unwrap(); + m1.iterator_mut(1).for_each(|v| *v += 1); + assert_eq!(m1.values, vec![2, 3, 4, 5]); + + // Case A: column-major, axis 0 (row-by-row) + let mut m2 = DenseMatrix::new(2, 2, vec![1, 2, 3, 4], true).unwrap(); + let vals: Vec = m2.iterator_mut(0).map(|v| *v).collect(); + assert_eq!(vals, vec![1, 3, 2, 4]); + m2.iterator_mut(0).for_each(|v| *v *= 2); + assert_eq!(m2.values, vec![2, 4, 6, 8]); + + // Case C: row-major, axis 0 (row-by-row) + let mut m3 = DenseMatrix::new(2, 2, vec![1, 2, 3, 4], false).unwrap(); + m3.iterator_mut(0).for_each(|v| *v += 1); + assert_eq!(m3.values, vec![2, 3, 4, 5]); + + // Case D: row-major, axis 1 (col-by-col) + let mut m4 = DenseMatrix::new(2, 2, vec![1, 2, 3, 4], false).unwrap(); + let vals: Vec = m4.iterator_mut(1).map(|v| *v).collect(); + assert_eq!(vals, vec![1, 3, 2, 4]); + m4.iterator_mut(1).for_each(|v| *v *= 2); + assert_eq!(m4.values, vec![2, 4, 6, 8]); + } + + #[test] + fn test_dense_matrix_mut_view_iter_mut_all_cases() { + // Case B: column-major, axis 1 (col-by-col) + let mut m1 = DenseMatrix::new(3, 3, (1..10).collect(), true).unwrap(); + { + let mut v = DenseMatrixMutView::new(&mut m1, 0..2, 0..2).unwrap(); + v.iter_mut(1).for_each(|v| *v = 0); + } + assert_eq!(m1.values, vec![0, 0, 3, 0, 0, 6, 7, 8, 9]); + + // Case A: column-major, axis 0 (row-by-row) + let mut m2 = DenseMatrix::new(3, 3, (1..10).collect(), true).unwrap(); + { + let mut v = DenseMatrixMutView::new(&mut m2, 0..2, 0..2).unwrap(); + let vals: Vec = v.iter_mut(0).map(|v| *v).collect(); + assert_eq!(vals, vec![1, 4, 2, 5]); + v.iter_mut(0).for_each(|v| *v = 0); + } + assert_eq!(m2.values, vec![0, 0, 3, 0, 0, 6, 7, 8, 9]); + + // Case C: row-major, axis 0 (row-by-row) + let mut m3 = DenseMatrix::new(3, 3, (1..10).collect(), false).unwrap(); + { + let mut v = DenseMatrixMutView::new(&mut m3, 0..2, 0..2).unwrap(); + v.iter_mut(0).for_each(|v| *v = 0); + } + assert_eq!(m3.values, vec![0, 0, 3, 0, 0, 6, 7, 8, 9]); + + // Case D: row-major, axis 1 (col-by-col) + let mut m4 = DenseMatrix::new(3, 3, (1..10).collect(), false).unwrap(); + { + let mut v = DenseMatrixMutView::new(&mut m4, 0..2, 0..2).unwrap(); + let vals: Vec = v.iter_mut(1).map(|v| *v).collect(); + assert_eq!(vals, vec![1, 4, 2, 5]); + v.iter_mut(1).for_each(|v| *v = 0); + } + assert_eq!(m4.values, vec![0, 0, 3, 0, 0, 6, 7, 8, 9]); + } + + #[test] + fn test_is_empty() { + let m = DenseMatrix::new(2, 2, vec![1, 2, 3, 4], true).unwrap(); + assert!(!m.is_empty()); + let empty: DenseMatrix = DenseMatrix::new(0, 0, vec![], true).unwrap(); + assert!(empty.is_empty()); + } + + #[test] + fn test_stride_range_error() { + let _m = DenseMatrix::new(2, 2, vec![1, 2, 3, 4], true).unwrap(); + } + + #[test] + #[should_panic(expected = "Invalid index (2,0) for 2x2 matrix")] + fn test_get_out_of_bounds() { + let m = DenseMatrix::from_2d_array(&[&[1, 2], &[3, 4]]).unwrap(); + m.get((2, 0)); + } + + #[test] + fn test_transpose_row_major() { + let m = DenseMatrix::new(2, 3, vec![1, 2, 3, 4, 5, 6], false).unwrap(); + let mt = m.transpose(); + assert!(mt.column_major); + assert_eq!(mt.nrows, 3); + assert_eq!(mt.ncols, 2); + assert_eq!(mt.values, vec![1, 2, 3, 4, 5, 6]); + } + + #[test] + #[should_panic(expected = "For two dimensional array `axis` should be either 0 or 1")] + fn test_iterator_invalid_axis() { + let m = DenseMatrix::from_2d_array(&[&[1, 2], &[3, 4]]).unwrap(); + let _ = m.iterator(2); + } + + #[test] + #[should_panic(expected = "For two dimensional array `axis` should be either 0 or 1")] + fn test_iterator_mut_invalid_axis() { + let mut m = DenseMatrix::from_2d_array(&[&[1, 2], &[3, 4]]).unwrap(); + let _ = m.iterator_mut(2); + } + + #[test] + fn test_view_1d_access() { + let m = DenseMatrix::from_2d_array(&[&[1, 2, 3], &[4, 5, 6]]).unwrap(); + let v_row = DenseMatrixView::new(&m, 0..1, 0..3).unwrap(); + assert_eq!( as Array>::shape(&v_row), 3); + assert_eq!( as Array>::get(&v_row, 1), &2); + + let v_col = DenseMatrixView::new(&m, 0..2, 1..2).unwrap(); + assert_eq!( as Array>::shape(&v_col), 2); + assert_eq!( as Array>::get(&v_col, 1), &5); + } + + #[test] + #[should_panic(expected = "This is neither a column nor a row")] + fn test_view_1d_access_invalid() { + let m = DenseMatrix::from_2d_array(&[&[1, 2, 3], &[4, 5, 6]]).unwrap(); + let v = DenseMatrixView::new(&m, 0..2, 0..2).unwrap(); + let _ = as Array>::shape(&v); } } From 5714314abb848ca38368e571acd11866ab548f17 Mon Sep 17 00:00:00 2001 From: jackpots28 Date: Thu, 21 May 2026 15:17:45 -0500 Subject: [PATCH 5/6] Failed to fix fmt before pushing --- src/linalg/basic/matrix.rs | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/linalg/basic/matrix.rs b/src/linalg/basic/matrix.rs index b70a4070..64d2f1ea 100644 --- a/src/linalg/basic/matrix.rs +++ b/src/linalg/basic/matrix.rs @@ -1073,7 +1073,7 @@ mod tests { &[1. + f32::EPSILON, 2., 3.], &[4., 5., 6. + f32::EPSILON], ]) - .unwrap(); + .unwrap(); let d = DenseMatrix::from_2d_array(&[&[1. + 0.5, 2., 3.], &[4., 5., 6. + f32::EPSILON]]) .unwrap(); @@ -1226,12 +1226,24 @@ mod tests { fn test_view_1d_access() { let m = DenseMatrix::from_2d_array(&[&[1, 2, 3], &[4, 5, 6]]).unwrap(); let v_row = DenseMatrixView::new(&m, 0..1, 0..3).unwrap(); - assert_eq!( as Array>::shape(&v_row), 3); - assert_eq!( as Array>::get(&v_row, 1), &2); + assert_eq!( + as Array>::shape(&v_row), + 3 + ); + assert_eq!( + as Array>::get(&v_row, 1), + &2 + ); let v_col = DenseMatrixView::new(&m, 0..2, 1..2).unwrap(); - assert_eq!( as Array>::shape(&v_col), 2); - assert_eq!( as Array>::get(&v_col, 1), &5); + assert_eq!( + as Array>::shape(&v_col), + 2 + ); + assert_eq!( + as Array>::get(&v_col, 1), + &5 + ); } #[test] From 7d968c4b08667914d1904b2f8d3bb8b76faa35f0 Mon Sep 17 00:00:00 2001 From: jackpots28 Date: Tue, 26 May 2026 09:43:17 -0500 Subject: [PATCH 6/6] Code Dedupe: DenseMatrix::iterator_mut and DenseMatrixMutView::iter_mut each contained their own copy of the strided/transposed mutable iteration logic -> Both now delegate to a single make_iter_mut free function, with strided_iter_mut and TransposedIterMut as shared building blocks High Level Overview for Optimization from_2d_array called from_2d_vec after allocating a temporary Vec> -> from_2d_array now validates and builds the column-major flat buffer directly from the input &[&[T]] slices - same logic as from_2d_vec (zero intermediate allocation) --- src/linalg/basic/matrix.rs | 546 +++++++++++++++++++++++-------------- 1 file changed, 343 insertions(+), 203 deletions(-) diff --git a/src/linalg/basic/matrix.rs b/src/linalg/basic/matrix.rs index 64d2f1ea..80f95d93 100644 --- a/src/linalg/basic/matrix.rs +++ b/src/linalg/basic/matrix.rs @@ -80,6 +80,12 @@ impl<'a, T: Debug + Display + Copy + Sized> DenseMatrixView<'a, T> { axis == 1 || axis == 0, "For two dimensional array `axis` should be either 0 or 1" ); + + let contiguous_row_major = !self.column_major && axis == 0 && self.stride == self.ncols; + let contiguous_col_major = self.column_major && axis == 1 && self.stride == self.nrows; + if contiguous_row_major || contiguous_col_major { + return Box::new(self.values.iter()); + } match axis { 0 => Box::new( (0..self.nrows).flat_map(move |r| (0..self.ncols).map(move |c| self.get((r, c)))), @@ -132,6 +138,15 @@ impl<'a, T: Debug + Display + Copy + Sized> DenseMatrixMutView<'a, T> { axis == 1 || axis == 0, "For two dimensional array `axis` should be either 0 or 1" ); + + // Fast path: when the view is contiguous in the requested traversal order, + // return a plain slice iterator with no per-element dispatch overhead. + let contiguous_row_major = !self.column_major && axis == 0 && self.stride == self.ncols; + let contiguous_col_major = self.column_major && axis == 1 && self.stride == self.nrows; + if contiguous_row_major || contiguous_col_major { + return Box::new(self.values.iter()); + } + match axis { 0 => Box::new( (0..self.nrows).flat_map(move |r| (0..self.ncols).map(move |c| self.get((r, c)))), @@ -143,6 +158,13 @@ impl<'a, T: Debug + Display + Copy + Sized> DenseMatrixMutView<'a, T> { } fn iter_mut<'b>(&'b mut self, axis: u8) -> Box + 'b> { + let nrows = self.nrows; + let ncols = self.ncols; + + if ncols == 0 || nrows == 0 { + return Box::new(std::iter::empty()); + } + assert!( axis == 1 || axis == 0, "For two dimensional array `axis` should be either 0 or 1" @@ -150,127 +172,129 @@ impl<'a, T: Debug + Display + Copy + Sized> DenseMatrixMutView<'a, T> { let column_major = self.column_major; let stride = self.stride; - let nrows = self.nrows; - let ncols = self.ncols; - // Axis = 0: row-by-row (outer loop over rows, inner over cols) - // Axis = 1: col-by-col (outer loop over cols, inner over rows) - // Four cases: column-major (axis 0 or 1), row-major (axis 1 or 0) + // Eagerly validate that each strided chunk is large enough before returning iterator + match (column_major, axis) { + (true, 1) => assert!( + stride >= nrows, + "iter_mut: chunk size ({}) < take ({}): view layout is inconsistent", + stride, + nrows + ), + (false, 0) => assert!( + stride >= ncols, + "iter_mut: chunk size ({}) < take ({}): view layout is inconsistent", + stride, + ncols + ), + _ => {} + } + + make_iter_mut(self.values, column_major, axis, stride, nrows, ncols) + } +} - // Collect all mutable references up-front using split_at_mut so - // that the resulting iterator owns no borrow of "self.values" +/// Shared mutable iterator logic for both `DenseMatrix::iterator_mut` and +/// `DenseMatrixMutView::iter_mut` +fn make_iter_mut<'a, T: Debug + Display + Copy + Sized>( + slice: &'a mut [T], + column_major: bool, + axis: u8, + stride: usize, + nrows: usize, + ncols: usize, +) -> Box + 'a> { + match (column_major, axis) { + // Case B: column-major, col-by-col + (true, 1) => Box::new(strided_iter_mut(slice, ncols, stride, nrows)), - match (column_major, axis) { - // Case B: column-major, col-by-col - (true, 1) => { - let mut refs: Vec<&'b mut T> = Vec::with_capacity(ncols * nrows); - let mut remaining: &'b mut [T] = self.values; - for _c in 0..ncols { - let col_end = if _c == ncols - 1 { - remaining.len() - } else { - stride - }; - let (col_slice, tail) = remaining.split_at_mut(col_end); - for elem in col_slice[..nrows].iter_mut() { - refs.push(elem); - } - remaining = tail; - } - Box::new(refs.into_iter()) - } + // Case A: column-major, row-by-row + (true, _) => Box::new(TransposedIterMut::new(slice, stride, nrows, ncols)), - // Case A: column-major, row-by-row - (true, _) => { - let mut refs: Vec<&'b mut T> = Vec::with_capacity(nrows * ncols); - - let total = nrows * ncols; - - let mut by_col: Vec<&'b mut T> = Vec::with_capacity(total); - { - let mut remaining: &'b mut [T] = self.values; - for _c in 0..ncols { - let col_end = if _c == ncols - 1 { - remaining.len() - } else { - stride - }; - let (col_slice, tail) = remaining.split_at_mut(col_end); - for elem in col_slice[..nrows].iter_mut() { - by_col.push(elem); - } - remaining = tail; - } - } + // Case C: row-major, row-by-row + (false, 0) => Box::new(strided_iter_mut(slice, nrows, stride, ncols)), + + // Case D: row-major, col-by-col + (false, _) => Box::new(TransposedIterMut::new(slice, stride, ncols, nrows)), + } +} + +/// Returns a lazy iterator over the first `take` elements of each +/// chunk of size `chunk_size` from `slice`, using `chunks_mut` + `flat_map` +/// to maintain lazy evaluation without any up-front allocation +fn strided_iter_mut( + slice: &mut [T], + _chunks: usize, + chunk_size: usize, + take: usize, +) -> impl Iterator { + slice.chunks_mut(chunk_size).flat_map(move |chunk| { + assert!( + chunk.len() >= take, + "iter_mut: chunk size ({}) < take ({}): view layout is inconsistent", + chunk.len(), + take + ); + chunk[..take].iter_mut() + }) +} - let mut indexed: Vec<(usize, &'b mut T)> = by_col - .into_iter() - .enumerate() - .map(|(flat_col_idx, r)| { - let c = flat_col_idx / nrows; - let row = flat_col_idx % nrows; - let out_idx = row * ncols + c; - (out_idx, r) - }) - .collect(); - indexed.sort_unstable_by_key(|(idx, _)| *idx); - refs.extend(indexed.into_iter().map(|(_, r)| r)); - Box::new(refs.into_iter()) +struct TransposedIterMut<'a, T> { + chunks: Vec<&'a mut [T]>, + outer_count: usize, + outer_idx: usize, + inner_idx: usize, +} + +impl<'a, T> TransposedIterMut<'a, T> { + fn new(slice: &'a mut [T], stride: usize, outer_count: usize, inner_count: usize) -> Self { + let mut chunks: Vec<&'a mut [T]> = Vec::with_capacity(inner_count); + let mut remaining = slice; + for i in 0..inner_count { + let chunk_len = if i < inner_count - 1 { + stride + } else { + remaining.len() + }; + let (head, tail) = remaining.split_at_mut(chunk_len); + chunks.push(head); + remaining = tail; + } + TransposedIterMut { + chunks, + outer_count, + outer_idx: 0, + inner_idx: 0, + } + } +} + +impl<'a, T> Iterator for TransposedIterMut<'a, T> { + type Item = &'a mut T; + + fn next(&mut self) -> Option { + loop { + if self.outer_idx >= self.outer_count { + return None; } - // Case C: row-major, row-by-row - (false, 0) => { - let mut refs: Vec<&'b mut T> = Vec::with_capacity(nrows * ncols); - let mut remaining: &'b mut [T] = self.values; - for _r in 0..nrows { - let row_end = if _r == nrows - 1 { - remaining.len() - } else { - stride - }; - let (row_slice, tail) = remaining.split_at_mut(row_end); - for elem in row_slice[..ncols].iter_mut() { - refs.push(elem); - } - remaining = tail; - } - Box::new(refs.into_iter()) + if self.inner_idx >= self.chunks.len() { + self.outer_idx += 1; + self.inner_idx = 0; + continue; } - // Case D: row-major, col-by-col - (false, _) => { - let total = nrows * ncols; - let mut by_row: Vec<&'b mut T> = Vec::with_capacity(total); - { - let mut remaining: &'b mut [T] = self.values; - for _r in 0..nrows { - let row_end = if _r == nrows - 1 { - remaining.len() - } else { - stride - }; - let (row_slice, tail) = remaining.split_at_mut(row_end); - for elem in row_slice[..ncols].iter_mut() { - by_row.push(elem); - } - remaining = tail; - } + let chunk: &'a mut [T] = std::mem::take(&mut self.chunks[self.inner_idx]); + match chunk.split_first_mut() { + Some((first, rest)) => { + self.chunks[self.inner_idx] = rest; + self.inner_idx += 1; + return Some(first); + } + None => { + // Empty chunk (shouldn't happen with valid inputs), skip it + self.inner_idx += 1; } - - let mut indexed: Vec<(usize, &'b mut T)> = by_row - .into_iter() - .enumerate() - .map(|(flat_row_idx, r)| { - let row = flat_row_idx / ncols; - let col = flat_row_idx % ncols; - let out_idx = col * nrows + row; - (out_idx, r) - }) - .collect(); - indexed.sort_unstable_by_key(|(idx, _)| *idx); - let mut refs: Vec<&'b mut T> = Vec::with_capacity(total); - refs.extend(indexed.into_iter().map(|(_, r)| r)); - Box::new(refs.into_iter()) } } } @@ -314,7 +338,33 @@ impl DenseMatrix { /// New instance of `DenseMatrix` from 2d array. pub fn from_2d_array(values: &[&[T]]) -> Result { - DenseMatrix::from_2d_vec(&values.iter().map(|row| Vec::from(*row)).collect()) + if values.is_empty() || values[0].is_empty() { + return Err(Failed::input( + "The 2d vec provided is empty; cannot instantiate the matrix", + )); + } + + let nrows = values.len(); + let ncols = values[0].len(); + + for (i, row) in values.iter().enumerate() { + if row.len() != ncols { + return Err(Failed::input(&format!( + "Row {i} has length {} but row 0 has length {ncols}; \ + jagged arrays are not supported", + row.len() + ))); + } + } + + let mut m_values = Vec::with_capacity(nrows * ncols); + for c in 0..ncols { + for r in values.iter() { + m_values.push(r[c]); + } + } + + DenseMatrix::new(nrows, ncols, m_values, true) } /// New instance of `DenseMatrix` from 2d vector. @@ -343,11 +393,16 @@ impl DenseMatrix { } } + // Build column-major storage: for each column, push that column's + // element from every row. Using a temporary row-major buffer and + // then transposing in bulk is not faster here because the input is + // already row-slices; we iterate column-by-column to match the + // column-major layout expected by `DenseMatrix::new(..., true)`. let mut m_values = Vec::with_capacity(nrows * ncols); for c in 0..ncols { - for r in values.iter().take(nrows) { - m_values.push(r[c]) + for r in values.iter() { + m_values.push(r[c]); } } @@ -359,6 +414,49 @@ impl DenseMatrix { self.values.iter() } + /// Returns the full backing slice of matrix values. + /// + /// The layout is determined by `column_major`: column-major if `true`, row-major if `false`. + /// Use this for zero-overhead bulk access when you know the storage order. + #[inline] + pub fn values_slice(&self) -> &[T] { + &self.values + } + + /// Returns a slice of the elements in row `row`. + /// + /// For row-major storage this is a single contiguous slice of `ncols` elements. + /// For column-major storage the elements are not contiguous, so `None` is returned. + #[inline] + pub fn row_slice(&self, row: usize) -> Option<&[T]> { + if row >= self.nrows { + return None; + } + if !self.column_major { + let start = row * self.ncols; + Some(&self.values[start..start + self.ncols]) + } else { + None + } + } + + /// Returns a slice of the elements in column `col`. + /// + /// For column-major storage this is a single contiguous slice of `nrows` elements. + /// For row-major storage the elements are not contiguous, so `None` is returned. + #[inline] + pub fn col_slice(&self, col: usize) -> Option<&[T]> { + if col >= self.ncols { + return None; + } + if self.column_major { + let start = col * self.nrows; + Some(&self.values[start..start + self.nrows]) + } else { + None + } + } + /// Check if the size of the requested view is bounded to matrix rows/cols count. fn is_valid_view( &self, @@ -392,17 +490,22 @@ impl DenseMatrix { .expect("stride_range: integer overflow in start (column_major)"), ) .expect("stride_range: integer overflow in start (column_major)"); - let end = vrows - .end - .checked_add( - vcols - .end - .checked_sub(1) - .expect("stride_range: vcols.end underflow (column_major)") - .checked_mul(n_rows) - .expect("stride_range: integer overflow in end (column_major)"), - ) - .expect("stride_range: integer overflow in end (column_major)"); + + let end = if vcols.is_empty() || vrows.is_empty() { + start + } else { + vrows + .end + .checked_add( + vcols + .end + .checked_sub(1) + .expect("stride_range: vcols.end underflow (column_major)") + .checked_mul(n_rows) + .expect("stride_range: integer overflow in end (column_major)"), + ) + .expect("stride_range: integer overflow in end (column_major)") + }; (start, end, n_rows) } else { let start = vrows @@ -411,14 +514,19 @@ impl DenseMatrix { .expect("stride_range: integer overflow in start (row_major)") .checked_add(vcols.start) .expect("stride_range: integer overflow in start (row_major)"); - let end = vrows - .end - .checked_sub(1) - .expect("stride_range: vrows.end underflow (row_major)") - .checked_mul(n_cols) - .expect("stride_range: integer overflow in end (row_major)") - .checked_add(vcols.end) - .expect("stride_range: integer overflow in end (row_major)"); + + let end = if vrows.is_empty() || vcols.is_empty() { + start + } else { + vrows + .end + .checked_sub(1) + .expect("stride_range: vrows.end underflow (row_major)") + .checked_mul(n_cols) + .expect("stride_range: integer overflow in end (row_major)") + .checked_add(vcols.end) + .expect("stride_range: integer overflow in end (row_major)") + }; (start, end, n_cols) }; (start, end, stride) @@ -535,6 +643,16 @@ impl Array for DenseMatrix axis == 1 || axis == 0, "For two dimensional array `axis` should be either 0 or 1" ); + + // Fast path: a non-view DenseMatrix is always fully contiguous + // column-major storage is default col-by-col (axis == 1) + // row-major storage is by default row-by-row (axis == 0) + // In both matching cases we can return a plain slice iterator + let natural_order = (self.column_major && axis == 1) || (!self.column_major && axis == 0); + if natural_order { + return Box::new(self.values.iter()); + } + match axis { 0 => Box::new( (0..self.nrows).flat_map(move |r| (0..self.ncols).map(move |c| self.get((r, c)))), @@ -556,85 +674,27 @@ impl MutArray for DenseMat } fn iterator_mut<'b>(&'b mut self, axis: u8) -> Box + 'b> { + let (nrows, ncols) = self.shape(); + + if ncols == 0 || nrows == 0 { + return Box::new(std::iter::empty()); + } + assert!( axis == 1 || axis == 0, "For two dimensional array `axis` should be either 0 or 1" ); let column_major = self.column_major; - let (nrows, ncols) = self.shape(); - - match (column_major, axis) { - // Case B: column-major, col-by-col - (true, 1) => { - let refs: Vec<&'b mut T> = self - .values - .chunks_mut(nrows) - .flat_map(|col| col.iter_mut()) - .collect(); - Box::new(refs.into_iter()) - } - // Case A: column-major, row-by-row - (true, _) => { - let total = nrows * ncols; - let by_col: Vec<&'b mut T> = self - .values - .chunks_mut(nrows) - .flat_map(|col| col.iter_mut()) - .collect(); - - let mut indexed: Vec<(usize, &'b mut T)> = by_col - .into_iter() - .enumerate() - .map(|(flat_col_idx, elem)| { - let c = flat_col_idx / nrows; - let r = flat_col_idx % nrows; - (r * ncols + c, elem) - }) - .collect(); - indexed.sort_unstable_by_key(|(idx, _)| *idx); - - let mut refs: Vec<&'b mut T> = Vec::with_capacity(total); - refs.extend(indexed.into_iter().map(|(_, e)| e)); - Box::new(refs.into_iter()) - } + let natural_order = (column_major && axis == 1) || (!column_major && axis == 0); + if natural_order { + return Box::new(self.values.iter_mut()); + } - // Case C: row-major, row-by-row - (false, 0) => { - let refs: Vec<&'b mut T> = self - .values - .chunks_mut(ncols) - .flat_map(|row| row.iter_mut()) - .collect(); - Box::new(refs.into_iter()) - } + let stride = if column_major { nrows } else { ncols }; - // Case D: row-major, col-by-col - (false, _) => { - let total = nrows * ncols; - let by_row: Vec<&'b mut T> = self - .values - .chunks_mut(ncols) - .flat_map(|row| row.iter_mut()) - .collect(); - - let mut indexed: Vec<(usize, &'b mut T)> = by_row - .into_iter() - .enumerate() - .map(|(flat_row_idx, elem)| { - let r = flat_row_idx / ncols; - let c = flat_row_idx % ncols; - (c * nrows + r, elem) - }) - .collect(); - indexed.sort_unstable_by_key(|(idx, _)| *idx); - - let mut refs: Vec<&'b mut T> = Vec::with_capacity(total); - refs.extend(indexed.into_iter().map(|(_, e)| e)); - Box::new(refs.into_iter()) - } - } + make_iter_mut(&mut self.values, column_major, axis, stride, nrows, ncols) } } @@ -1139,6 +1199,86 @@ mod tests { assert_eq!(m4.values, vec![2, 4, 6, 8]); } + #[test] + fn test_iter_empty_matrix() { + let m00: DenseMatrix = DenseMatrix::new(0, 0, vec![], true).unwrap(); + assert_eq!(m00.iterator(0).count(), 0); + assert_eq!(m00.iterator(1).count(), 0); + + let m05: DenseMatrix = DenseMatrix::new(0, 5, vec![], true).unwrap(); + assert_eq!(m05.iterator(0).count(), 0); + assert_eq!(m05.iterator(1).count(), 0); + + let m50: DenseMatrix = DenseMatrix::new(5, 0, vec![], true).unwrap(); + assert_eq!(m50.iterator(0).count(), 0); + assert_eq!(m50.iterator(1).count(), 0); + } + + #[test] + fn test_iterator_mut_empty_matrix() { + let mut m00: DenseMatrix = DenseMatrix::new(0, 0, vec![], true).unwrap(); + assert_eq!(m00.iterator_mut(0).count(), 0); + assert_eq!(m00.iterator_mut(1).count(), 0); + + let mut m05: DenseMatrix = DenseMatrix::new(0, 5, vec![], true).unwrap(); + assert_eq!(m05.iterator_mut(0).count(), 0); + assert_eq!(m05.iterator_mut(1).count(), 0); + + let mut m50: DenseMatrix = DenseMatrix::new(5, 0, vec![], true).unwrap(); + assert_eq!(m50.iterator_mut(0).count(), 0); + assert_eq!(m50.iterator_mut(1).count(), 0); + } + + #[test] + fn test_iter_mut_view_empty_matrix() { + let mut m: DenseMatrix = DenseMatrix::fill(5, 5, 0.0); + // Create an empty view + let mut v = DenseMatrixMutView::new(&mut m, 0..0, 0..5).unwrap(); + assert_eq!(v.iter_mut(0).count(), 0); + assert_eq!(v.iter_mut(1).count(), 0); + + let mut v2 = DenseMatrixMutView::new(&mut m, 0..5, 0..0).unwrap(); + assert_eq!(v2.iter_mut(0).count(), 0); + assert_eq!(v2.iter_mut(1).count(), 0); + } + + #[test] + fn test_iter_single_row_column() { + let m13 = DenseMatrix::from_2d_array(&[&[1.0, 2.0, 3.0]]).unwrap(); + assert_eq!( + m13.iterator(0).cloned().collect::>(), + vec![1.0, 2.0, 3.0] + ); + assert_eq!( + m13.iterator(1).cloned().collect::>(), + vec![1.0, 2.0, 3.0] + ); + + let m31 = DenseMatrix::from_2d_array(&[&[1.0], &[2.0], &[3.0]]).unwrap(); + assert_eq!( + m31.iterator(0).cloned().collect::>(), + vec![1.0, 2.0, 3.0] + ); + assert_eq!( + m31.iterator(1).cloned().collect::>(), + vec![1.0, 2.0, 3.0] + ); + } + + #[test] + #[should_panic(expected = "iter_mut: chunk size (2) < take (3)")] + fn test_iter_mut_stride_validation() { + let mut values = vec![1.0, 2.0, 3.0, 4.0]; + let mut view = DenseMatrixMutView { + values: &mut values, + stride: 2, + nrows: 3, // take 3 from chunk of size 2 - should panic + ncols: 2, // at least 2 columns to trigger chunking + column_major: true, + }; + let _ = view.iter_mut(1); + } + #[test] fn test_dense_matrix_mut_view_iter_mut_all_cases() { // Case B: column-major, axis 1 (col-by-col)