Implement the Default trait for VecStorage, CsMatrix, CscMatrix, etc. #1206
Conversation
This commit solves dimforge#1204 and implements the Default trait for CsMatrix, CscMatrix, SparsityPattern, VecStorage.
There was a problem hiding this comment.
Thanks for this PR, I agree that having more Default impls would make life easier, and I regularly run into the lack of these myself.
I'd prefer to have some sanity tests for the Default impls where you use the "try_from..." constructors to validate the output against what you expected. The current impl of CsMatrix is unsound and would be caught by such a test, see comments.
For the VecStorage impl I have an alternative suggestion that I think would be more practically useful, please see comment there as well.
| @@ -22,6 +22,16 @@ pub struct CsMatrix<T> { | |||
| values: Vec<T>, | |||
| } | |||
|
|
|||
| ///The default implementation for the CsMatrix | |||
There was a problem hiding this comment.
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.
| Self { | ||
| sparsity_pattern: Default::default(), | ||
| values: Default::default(), | ||
| } |
There was a problem hiding this comment.
I'd prefer to be more explicit here, i.e. vec![] instead.
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
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 requireT: DefaultR: DimName,C = Dynamic. In this case we can avoid requiringT: Defaultand create an "empty" matrix.R = Dynamic,C: DimName. Same as the previous.R = Dynamic,C = Dynamic. Same, we don't needT: 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.
There was a problem hiding this comment.
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.
| major_offsets: Default::default(), | ||
| minor_indices: Default::default(), | ||
| minor_dim: Default::default(), | ||
| } |
There was a problem hiding this comment.
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
}
|
It looks as if these changes with Andlon's recommendations were implemented in #1312 , so I'll close this pull request. |
This commit solves #1204 and implements the Default trait for CsMatrix, CscMatrix, SparsityPattern, VecStorage.