-
Notifications
You must be signed in to change notification settings - Fork 100
Add DenseMatrix constructor for owned ndarray matrices #381
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
base: development
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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) | ||
| } | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion 4 — Magic The Please introduce a named constant at the top of the // 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)
}
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
| { | ||
|
|
@@ -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); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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]]); | ||
|
|
||
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.
[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 aliasndarray::Array2<T>is cleaner and is what users will have in scope:Note:
ndarray::Array2<T>is simply a type alias forArrayBase<OwnedRepr<T>, Ix2>, so this is a zero-cost, non-breaking change.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.
Addressed in d4bc985. The public parameter now uses ndarray::Array2.