Add DenseMatrix constructor for owned ndarray matrices#381
Conversation
|
Thanks for this! I have started the CI and I will provide a code review asap. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development #381 +/- ##
===============================================
- Coverage 45.59% 44.29% -1.30%
===============================================
Files 93 94 +1
Lines 8034 8061 +27
===============================================
- Hits 3663 3571 -92
- Misses 4371 4490 +119 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Mec-iS
left a comment
Mec-iS
left a comment
There was a problem hiding this comment.
Code Review: DenseMatrix::from_ndarray2
Thank you for this addition — the feature fills a real gap and the transposed-layout regression test is a great catch. After reviewing the diff against the DenseMatrix internals I have 5 actionable suggestions before merging, detailed as inline comments below.
Summary
| # | Area | Severity |
|---|---|---|
| 1 | API: use Array2<T> alias instead of ArrayBase<OwnedRepr<T>, Ix2> |
Minor |
| 2 | Trait bounds: redundant Sized |
Nit |
| 3 | Magic 0 axis literal unexplained |
Minor |
| 4 | Doc-comment missing # Panics and # Notes (layout invariant) |
Minor |
| 5 | Test coverage missing edge cases | Minor |
Suggested replacement for the entire new block
// `axis = 0` passed to DenseMatrix::from_iterator means row-major storage
// (column_major = false). Named here to avoid a magic literal.
const ROW_MAJOR_AXIS: u8 = 0;
impl<T: Debug + Display + Copy> DenseMatrix<T> {
/// Copies an owned two-dimensional ndarray into a [`DenseMatrix`].
///
/// The resulting matrix uses row-major (C) storage regardless of the
/// memory layout of the source array.
///
/// # Notes
///
/// [`ndarray::Array2::iter`] always yields elements in **logical
/// row-major order** (row 0 col 0, row 0 col 1, …, row n col m),
/// independent of whether the source array is stored in C or Fortran
/// (column-major) order. This is what makes the transposed-axis test
/// pass correctly and should be preserved by future refactors.
///
/// # Panics
///
/// Panics if `nrows * ncols` overflows `usize`, which is unreachable on
/// 32- and 64-bit targets in practice. An empty (0 rows or 0 cols)
/// array produces an empty `DenseMatrix` without panicking.
///
/// # Examples
///
/// ```
/// use ndarray::Array2;
/// use smartcore::linalg::basic::matrix::DenseMatrix;
///
/// // Build a 3×4 ndarray from a flat vector and convert it.
/// let nd: Array2<f64> = Array2::from_shape_vec(
/// (3, 4),
/// (0..12).map(|x| x as f64).collect(),
/// ).unwrap();
/// let dm = DenseMatrix::from_ndarray2(&nd);
/// assert_eq!(dm.shape(), (3, 4));
/// assert_eq!(*dm.get((1, 2)), 6.0);
/// ```
pub fn from_ndarray2(a: &ndarray::Array2<T>) -> Self {
// .iter() yields elements in logical row-major order regardless of
// the underlying memory layout, so ROW_MAJOR_AXIS is always correct.
Self::from_iterator(a.iter().copied(), a.nrows(), a.ncols(), ROW_MAJOR_AXIS)
}
}Suggested additional tests
#[test]
fn test_dense_matrix_from_ndarray2_square() {
let input = arr2(&[[1, 2], [3, 4]]);
let matrix = DenseMatrix::from_ndarray2(&input);
let expected = DenseMatrix::from_2d_array(&[&[1, 2], &[3, 4]]).unwrap();
assert_eq!(matrix, expected);
}
#[test]
fn test_dense_matrix_from_ndarray2_row_vector() {
let input = arr2(&[[10, 20, 30, 40]]);
let matrix = DenseMatrix::from_ndarray2(&input);
let expected = DenseMatrix::from_2d_array(&[&[10, 20, 30, 40]]).unwrap();
assert_eq!(matrix, expected);
assert_eq!(matrix.shape(), (1, 4));
}
#[test]
fn test_dense_matrix_from_ndarray2_col_vector() {
let input = arr2(&[[10], [20], [30], [40]]);
let matrix = DenseMatrix::from_ndarray2(&input);
let expected = DenseMatrix::from_2d_array(&[&[10], &[20], &[30], &[40]]).unwrap();
assert_eq!(matrix, expected);
assert_eq!(matrix.shape(), (4, 1));
}
#[test]
fn test_dense_matrix_from_ndarray2_empty() {
// 0×0 empty array — must not panic
let input = ndarray::Array2::<i32>::zeros((0, 0));
let matrix = DenseMatrix::from_ndarray2(&input);
assert!(matrix.is_empty());
assert_eq!(matrix.shape(), (0, 0));
}All other existing tests and logic in the file are correct and should remain untouched.
Mec-iS
left a comment
Mec-iS
left a comment
There was a problem hiding this comment.
Inline comment batch — see individual comments below for each suggestion location.
|
please check the linting, see all the commands to run in CONTRIBUTING.md |
Mec-iS
left a comment
Mec-iS
left a comment
There was a problem hiding this comment.
Posting 5 targeted inline comments (line-level) to accompany the earlier review. Each comment maps directly to one of the 5 suggestions in the REQUEST_CHANGES review above.
| // ArrayBase<OwnedRepr<T>, Ix2> (owned 2-D array) | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| impl<T: Debug + Display + Copy + Sized> DenseMatrix<T> { |
There was a problem hiding this comment.
[Suggestion 1 — Redundant Sized bound]
Sized is implicit on every generic type parameter in Rust and should be removed to reduce noise:
// before
impl<T: Debug + Display + Copy + Sized> DenseMatrix<T> {
// after
impl<T: Debug + Display + Copy> DenseMatrix<T> {| // --------------------------------------------------------------------------- | ||
|
|
||
| impl<T: Debug + Display + Copy + Sized> DenseMatrix<T> { | ||
| /// Copies an owned two-dimensional ndarray into a `DenseMatrix`. |
There was a problem hiding this comment.
[Suggestion 2 — Doc-comment: missing # Panics and # Notes]
The current doc-comment only has # Examples. Please add:
- A
# Notessection documenting the critical layout invariant:ndarray::iter()always yields elements in logical row-major order regardless of whether the source array is C- or Fortran-ordered. This is what makes the transposed-axis test pass and must not be silently removed in future refactors. - A
# Panicssection: panics ifnrows * ncolsoverflowsusize(unreachable on 32/64-bit targets). An empty (0 rows or 0 cols) array does not panic.
Also, consider replacing the arr2 example with a slightly more realistic use-case (e.g. Array2::from_shape_vec) to differentiate it from the unit test below.
/// Copies an owned two-dimensional ndarray into a [`DenseMatrix`].
///
/// The resulting matrix uses row-major (C) storage regardless of the
/// memory layout of the source array.
///
/// # Notes
///
/// [`ndarray::Array2::iter`] always yields elements in **logical
/// row-major order** (row 0 col 0, row 0 col 1, …), independent of
/// whether the source is C- or Fortran-ordered. This is the invariant
/// that makes transposed-layout conversion correct here.
///
/// # Panics
///
/// Panics if `nrows * ncols` overflows `usize` (unreachable on 32/64-bit
/// targets). An empty (0 rows or 0 cols) array does not panic.
///
/// # Examples
///
/// ```
/// use ndarray::Array2;
/// use smartcore::linalg::basic::matrix::DenseMatrix;
///
/// let nd: Array2<f64> = Array2::from_shape_vec(
/// (3, 4),
/// (0..12).map(|x| x as f64).collect(),
/// ).unwrap();
/// let dm = DenseMatrix::from_ndarray2(&nd);
/// assert_eq!(dm.shape(), (3, 4));
/// assert_eq!(*dm.get((1, 2)), 6.0);
/// ```
| /// ``` | ||
| pub fn from_ndarray2(a: &ArrayBase<OwnedRepr<T>, Ix2>) -> Self { | ||
| Self::from_iterator(a.iter().copied(), a.nrows(), a.ncols(), 0) | ||
| } |
There was a problem hiding this comment.
[Suggestion 3 — API signature: prefer Array2<T> alias]
The public signature exposes ArrayBase<OwnedRepr<T>, Ix2> which leaks ndarray’s internal generic machinery. The canonical public alias ndarray::Array2<T> is cleaner and is what users will have in scope:
// before
pub fn from_ndarray2(a: &ArrayBase<OwnedRepr<T>, Ix2>) -> Self {
// after
pub fn from_ndarray2(a: &ndarray::Array2<T>) -> Self {Note: ndarray::Array2<T> is simply a type alias for ArrayBase<OwnedRepr<T>, Ix2>, so this is a zero-cost, non-breaking change.
| pub fn from_ndarray2(a: &ArrayBase<OwnedRepr<T>, Ix2>) -> Self { | ||
| Self::from_iterator(a.iter().copied(), a.nrows(), a.ncols(), 0) | ||
| } | ||
| } |
There was a problem hiding this comment.
[Suggestion 4 — Magic 0 axis literal]
The 0 here is opaque without context. After checking the entire codebase, axis = 0 means row-major storage (column_major = false) in DenseMatrix::from_iterator — it is the universal convention used in dbscan.rs, csv.rs, logistic_regression.rs and elsewhere.
Please introduce a named constant at the top of the impl block (or file scope) and add an inline comment:
// At file/module scope:
const ROW_MAJOR_AXIS: u8 = 0;
// In the method body:
pub fn from_ndarray2(a: &ndarray::Array2<T>) -> Self {
// .iter() yields logical row-major order regardless of memory layout,
// so ROW_MAJOR_AXIS (= 0, i.e. column_major=false) is always correct.
Self::from_iterator(a.iter().copied(), a.nrows(), a.ncols(), ROW_MAJOR_AXIS)
}| let transposed = input.reversed_axes(); | ||
| let matrix = DenseMatrix::from_ndarray2(&transposed); | ||
| let expected = DenseMatrix::from_2d_array(&[&[1, 4], &[2, 5], &[3, 6]]).unwrap(); | ||
| assert_eq!(matrix, expected); |
There was a problem hiding this comment.
[Suggestion 5 — Missing edge-case tests]
The current test covers a rectangular array and a transposed (Fortran-order) layout — both good. The following edge cases are missing:
#[test]
fn test_dense_matrix_from_ndarray2_square() {
// Square matrix (nrows == ncols)
let input = arr2(&[[1, 2], [3, 4]]);
let matrix = DenseMatrix::from_ndarray2(&input);
let expected = DenseMatrix::from_2d_array(&[&[1, 2], &[3, 4]]).unwrap();
assert_eq!(matrix, expected);
}
#[test]
fn test_dense_matrix_from_ndarray2_row_vector() {
// 1×N degenerate shape
let input = arr2(&[[10, 20, 30, 40]]);
let matrix = DenseMatrix::from_ndarray2(&input);
let expected = DenseMatrix::from_2d_array(&[&[10, 20, 30, 40]]).unwrap();
assert_eq!(matrix, expected);
assert_eq!(matrix.shape(), (1, 4));
}
#[test]
fn test_dense_matrix_from_ndarray2_col_vector() {
// N×1 degenerate shape
let input = arr2(&[[10], [20], [30], [40]]);
let matrix = DenseMatrix::from_ndarray2(&input);
let expected = DenseMatrix::from_2d_array(&[&[10], &[20], &[30], &[40]]).unwrap();
assert_eq!(matrix, expected);
assert_eq!(matrix.shape(), (4, 1));
}
#[test]
fn test_dense_matrix_from_ndarray2_empty() {
// 0×0 empty array — must not panic
let input = ndarray::Array2::<i32>::zeros((0, 0));
let matrix = DenseMatrix::from_ndarray2(&input);
assert!(matrix.is_empty());
assert_eq!(matrix.shape(), (0, 0));
}2 participants
Fixes #326
Checklist
Current behaviour
Converting an owned two-dimensional ndarray into a
DenseMatrixrequires callers to reproduce the iterator, shape, and axis conversion manually.New expected behaviour
When the
ndarray-bindingsfeature is enabled,DenseMatrix::from_ndarray2copies an owned two-dimensional ndarray while preserving its logical row and column order. The regression test covers both a rectangular array and an owned array with transposed layout.Change logs
Added
DenseMatrix::from_ndarray2for owned two-dimensional ndarrays.Validation
cargo build --all-featurescargo build --no-default-featurescargo test --all-featurescargo test --no-default-features --features ndarray-bindingscargo fmt --all -- --checkcargo doc --all-features --no-depsThe strict clippy command reaches one existing
clippy::for_kv_mapfinding insrc/algorithm/neighbour/fastpair.rs:149on Rust 1.97.0. The same finding occurs on the unchanged development branch; allowing only that baseline lint leaves the all-features clippy run clean.