Skip to content
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.

Sign up for GitHub

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

Implement the Default trait for VecStorage, CsMatrix, CscMatrix, etc. #1206

Closed
wants to merge 1 commit into from
Closed

Implement the Default trait for VecStorage, CsMatrix, CscMatrix, etc. #1206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 10 additions & 0 deletions nalgebra-sparse/src/cs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,16 @@ pub struct CsMatrix<T> {
values: Vec<T>,
}

///The default implementation for the CsMatrix
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest to just remove this doc comment, since it doesn't add anything of value and might get outdated in the future (say if we rename CsMatrix or something like that).

Same for the other default impls.

impl<T> Default for CsMatrix<T> {
fn default() -> Self {
Self {
sparsity_pattern: Default::default(),
values: Default::default(),
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer to be more explicit here, i.e. vec![] instead.

}
}

impl<T> CsMatrix<T> {
/// Create a zero matrix with no explicitly stored entries.
#[inline]
Expand Down
9 changes: 9 additions & 0 deletions nalgebra-sparse/src/csc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,15 @@ pub struct CscMatrix<T> {
pub(crate) cs: CsMatrix<T>,
}

///The default implementation for the CscMatrix
impl<T> Default for CscMatrix<T> {
fn default() -> Self {
Self {
cs: Default::default(),
}
}
}

impl<T> CscMatrix<T> {
/// Constructs a CSC representation of the (square) `n x n` identity matrix.
#[inline]
Expand Down
11 changes: 11 additions & 0 deletions nalgebra-sparse/src/pattern.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,17 @@ pub struct SparsityPattern {
minor_dim: usize,
}

///The default implementation for the SparsityPattern
impl Default for SparsityPattern {
fn default() -> Self {
Self {
major_offsets: Default::default(),
minor_indices: Default::default(),
minor_dim: Default::default(),
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is incorrect since major_offsets should have length major_dim + 1. An "empty" pattern would look like:

Self {
  major_offsets: vec![0],
  minor_indices: vec![],
  minor_dim: 0
}

}
}

impl SparsityPattern {
/// Create a sparsity pattern of the given dimensions without explicitly stored entries.
pub fn zeros(major_dim: usize, minor_dim: usize) -> Self {
Expand Down
14 changes: 14 additions & 0 deletions src/base/vec_storage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,20 @@ pub struct VecStorage<T, R: Dim, C: Dim> {
ncols: C,
}

/// The default implementation for the VecStorage struct.
/// Requires R and C to implement the default trait.
impl<T, R: Dim + std::default::Default, C: Dim + std::default::Default> Default
for VecStorage<T, R, C>
{
fn default() -> Self {
Self {
data: Default::default(),
nrows: Default::default(),
ncols: Default::default(),
}
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think any dimensions actually implement Default at the moment 🤔

I think an alternative implementation could divide this into several cases:

  • R: DimName, C: DimName. In this case we need to require T: Default
  • R: DimName, C = Dynamic. In this case we can avoid requiring T: Default and create an "empty" matrix.
  • R = Dynamic, C: DimName. Same as the previous.
  • R = Dynamic, C = Dynamic. Same, we don't need T: Default.

This makes it possible to do

#[derive(Default)]
struct Wrapper<T> {
   vector: DVector<T>,
   matrix: DMatrix<T>,
}

without requiring T: Default. This is a common problem I'm frequently running into, at least.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note also that the proposed impl is unsound, because it always generates an empty vector regardless of whatever the dimensions are. This would lead to unsoundness whenever the dimensions are non-zero and any operation in nalgebra would try to index into the matrix.

#[cfg(feature = "serde-serialize")]
impl<T, R: Dim, C: Dim> Serialize for VecStorage<T, R, C>
where
Expand Down