dimforge / dimforge/nalgebra

Code comment Inconsistencies and wrong error messages

Open
#1,526 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
4.8k
Forks
565
PR merge metrics
No merged PRs in 30d

Description

In nalgebra-main/nalgebra-sparse/src/csc.rs, these 2 functions' error messages are "Row index must be in bounds", which should be "Column index must be in bounds".
```rust
/// Panics
/// ------
/// Panics if column index is out of bounds.
#[inline]
#[must_use]
pub fn col(&self, index: usize) -> CscCol<'_, T> {
self.get_col(index).expect("Row index must be in bounds")
}

/// Mutable column access for the given column index.
///
/// Panics
/// ------
/// Panics if column index is out of bounds.
#[inline]
pub fn col_mut(&mut self, index: usize) -> CscColMut<'_, T> {
self.get_col_mut(index)
.expect("Row index must be in bounds")
}
```
In nalgebra-main/nalgebra-sparse/src/factorization/cholesky.rs,
For those 2 functions, the comment indicates code will panic when values.len() != the number of non-zeros in the sparsity pattern, while actually code will not panic.
```rust
/// # Panics
///
/// Panics if the number of values differ from the number of non-zeros of the sparsity pattern
/// of the matrix that was symbolically factored.
pub fn factor_numerical(
symbolic: CscSymbolicCholesky,
values: &[T],
) -> Result {
...
}
/// # Panics
///
/// Panics if the number of values does not match the number of non-zeros in the sparsity
/// pattern.
pub fn refactor(&mut self, values: &[T]) -> Result<(), CholeskyError> {
self.decompose_left_looking(values)
}
```

Following test failed(not panic) when values.len()=2 and symbolic.m_pattern.nnz()=1
```rust
#[test]
#[should_panic]
fn test(){
let pattern = unsafe {
SparsityPattern::from_offset_and_indices_unchecked(1, 1, vec![0, 1], vec![0])
};
let symbolic = CscSymbolicCholesky::factor(pattern);
let values = [1.0, 2.0];
CscCholesky::::factor_numerical(symbolic, &values).unwrap();
}
```

For those 2 functions, the comment indicates that b must be square otherwise will panic, but actually code will not.
```rust
/// # Panics
///
/// Panics if `B` is not square.
#[must_use = "Did you mean to use solve_mut()?"]
pub fn solve<'a>(&'a self, b: impl Into>) -> DMatrix {
let b = b.into();
let mut output = b.clone_owned();
self.solve_mut(&mut output);
output
}

/// Solves the system `AX = B`, where `X` and `B` are dense matrices.
///
/// The result is stored in-place in `b`.
///
/// # Panics
///
/// Panics if `b` is not square.
pub fn solve_mut<'a>(&'a self, b: impl Into>) {
let expect_msg = "If the Cholesky factorization succeeded,\
then the triangular solve should never fail";
// Solve LY = B
let mut y = b.into();
spsolve_csc_lower_triangular(Op::NoOp(self.l()), &mut y).expect(expect_msg);

// Solve L^T X = Y
let mut x = y;
spsolve_csc_lower_triangular(Op::Transpose(self.l()), &mut x).expect(expect_msg);
}
```
Following test failed(not panic) while b is not square(b.nrows() = 1, b.ncols() = 2);
```rust
#[test]
#[should_panic]
fn test(){
let matrix = CscMatrix::::identity(1);
let chol = CscCholesky::::factor(&matrix).unwrap();
let mut b = nalgebra::DMatrix::::zeros(1, 2);
chol.solve_mut(&mut b);
}
```

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.