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.

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

Added into_vec method for Matrix #3

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from
24 changes: 24 additions & 0 deletions src/base/matrix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2357,3 +2357,27 @@ impl<T> super::alias::Matrix1<T> {
scalar
}
}

Copy link
Author

Choose a reason for hiding this comment

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

Contribution starts here

alexandruradovici marked this conversation as resolved.
Show resolved Hide resolved
/// Provides methods for transforming a matrix into a vector with different algorithms
impl<T, R: Dim, C: Dim, S: Storage<T, R, C>> Matrix<T, R, C, S>
where
T: Clone,
{
/// Converts matrix into a vector by concatenating rows
pub fn into_vec(&self) -> Vec<T> {
let (num_rows, num_columns) = self.shape();
let mut resulted_vector = Vec::with_capacity(num_rows * num_columns);

for i in 0..num_rows {
for j in 0..num_columns {
unsafe {
resulted_vector.push(self.get_unchecked((i, j)).clone());
}
Copy link

@alexandruradovici alexandruradovici Jan 23, 2024

Choose a reason for hiding this comment

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

Suggested change
unsafe {
resulted_vector.push(self.get_unchecked((i, j)).clone());
}
resulted_vector.push(unsafe { self.get_unchecked((i, j)) }.clone());

Please explain why did you use unsafe and why it is safe to do so.

Copy link
Author

Choose a reason for hiding this comment

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

As the method suggests (get_unchecked), there is no warranty that (i, j) are in bounds, that the operation does not fail or that the operation is memory safe. Now, the methods called by resulted_vector are not unsafe. If the resulted_vector is immutable, then it will result in compilation error, not that the operation is not memory safe. If the push exceeds vector capacity, then Rust is designed to reallocate space. I will modify.

Copy link
Author

Choose a reason for hiding this comment

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

I observed that was the way transpose method access the elements of a matrix in matrix.rs so I wanted to maintain the format.

Choose a reason for hiding this comment

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

Please add a comment in the code explaining why this is used and why it is safe.

Copy link
Author

Choose a reason for hiding this comment

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

I've added two comments that should clarify the problem

}
}

return resulted_vector;
alexandruradovici marked this conversation as resolved.
Show resolved Hide resolved
}
}