Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 88 additions & 0 deletions src/linalg/ndarray/matrix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use std::ops::Range;
use crate::linalg::basic::arrays::{
Array as BaseArray, Array2, ArrayView1, ArrayView2, MutArray, MutArrayView2,
};
use crate::linalg::basic::matrix::DenseMatrix;

use crate::linalg::traits::cholesky::CholeskyDecomposable;
use crate::linalg::traits::evd::EVDDecomposable;
Expand All @@ -19,6 +20,46 @@ use ndarray::{s, Array, ArrayBase, ArrayView, ArrayViewMut, Axis, Ix2, Order, Ow
// ArrayBase<OwnedRepr<T>, Ix2> (owned 2-D array)
// ---------------------------------------------------------------------------

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, independent of whether the source is C- or Fortran-ordered. This
/// invariant makes transposed-layout conversion correct.
///
/// # Panics
///
/// Panics if `nrows * ncols` overflows `usize`. An empty array (zero
/// rows or zero columns) does not panic.
///
/// # Examples
///
/// ```
/// use ndarray::Array2;
/// use smartcore::linalg::basic::arrays::Array;
/// use smartcore::linalg::basic::matrix::DenseMatrix;
///
/// let array = Array2::from_shape_vec(
/// (3, 4),
/// (0..12).map(|value| value as f64).collect(),
/// ).unwrap();
/// let matrix = DenseMatrix::from_ndarray2(&array);
/// assert_eq!(matrix.shape(), (3, 4));
/// assert_eq!(*matrix.get((1, 2)), 6.0);
/// ```
pub fn from_ndarray2(a: &ndarray::Array2<T>) -> Self {
// iter() yields logical row-major order regardless of memory layout.
Self::from_iterator(a.iter().copied(), a.nrows(), a.ncols(), ROW_MAJOR_AXIS)
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in d4bc985. The public parameter now uses ndarray::Array2.

}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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)
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in d4bc985. The row-major axis is now a named constant with an explanatory iterator comment.


impl<T: Debug + Display + Copy + Sized> BaseArray<T, (usize, usize)>
for ArrayBase<OwnedRepr<T>, Ix2>
{
Expand Down Expand Up @@ -224,6 +265,53 @@ mod tests {
use super::*;
use ndarray::arr2;

#[test]
fn test_dense_matrix_from_ndarray2() {
let input = arr2(&[[1, 2, 3], [4, 5, 6]]);
let matrix = DenseMatrix::from_ndarray2(&input);
let expected = DenseMatrix::from_2d_array(&[&[1, 2, 3], &[4, 5, 6]]).unwrap();
assert_eq!(matrix, expected);

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);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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));
}

}

#[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() {
let input = ndarray::Array2::<i32>::zeros((0, 0));
let matrix = DenseMatrix::from_ndarray2(&input);
assert!(matrix.is_empty());
assert_eq!(matrix.shape(), (0, 0));
}

#[test]
fn test_get_row() {
let m = arr2(&[[1, 2, 3], [4, 5, 6]]);
Expand Down
Loading