MatZ Mul, Cmp, Transpose by jnsiemer · Pull Request #115 · qfall/math

jnsiemer · 2023-03-27T15:29:19Z

This PR implements

Compare
Tranpose
Multiplication
including tests for MatZ.

Marvin-Beckmann · 2023-03-27T15:52:48Z

src/integer/mat_z/mul.rs

+    type Output = MatZ;
+
+    /// Implements the [`Mul`] trait for two [`MatZ`] values.
+    /// [`Mul`] is implemented for any combination of owned and borrowed [`MatZ`] and any kind of [`VecZ`].


VecZ?

Suggested change

/// [`Mul`] is implemented for any combination of owned and borrowed [`MatZ`] and any kind of [`VecZ`].

/// [`Mul`] is implemented for any combination of owned and borrowed [`MatZ`].

This doc-comment is correct. It's about all the combinations of Multiplication of that type that are implemented.

Please change

src/integer/mat_z/mul.rs

src/integer/mat_z/transpose.rs

Marvin-Beckmann · 2023-03-28T13:08:43Z

src/integer/mat_z/cmp.rs

+
+    use super::MatZ;
+    use std::str::FromStr;
+


the tests are all 2x2 matrices, please also add one for different sizes

Now one 3x3 test was included

Marvin-Beckmann · 2023-03-28T13:12:11Z

src/integer/vec_z/from.rs

+    /// Creates a new row-vector with `num_rows` rows and
+    /// zeros as entries.
+    ///
+    /// Parameters:
+    /// - `num_rows`: number of columns the new vector should have
+    ///
+    /// Returns a [`VecZ`] or an error, if the number of rows is
+    /// less or equal to `0`.
+    ///
+    /// # Example
+    /// ```
+    /// use math::integer::VecZ;
+    /// use math::utils::VectorDirection;
+    ///
+    /// let vector = VecZ::new(5, VectorDirection::ColumnVector).unwrap();
+    /// ```
+    ///
+    /// # Errors and Failures
+    /// - Returns a [`MathError`] of type
+    /// [`InvalidMatrix`](MathError::InvalidMatrix)
+    /// if the number of rows is `0`.
+    /// - Returns a [`MathError`] of type [`OutOfBounds`](MathError::OutOfBounds)
+    /// if the number of rows is negative or it does not fit into an [`i64`].


Does not fit to definition with vector direction

Now updated

Marvin-Beckmann · 2023-03-28T13:13:02Z

src/integer/vec_z/from.rs

+    /// Creates a [`VecZ`] row-vector with entries in [`Z`] from a [`String`].
+    /// The format of that string looks like this `[1,2,3]` for a row vector
+    /// with three entries (`1` in the first row, `2` in the second one, ...)


does not fit to direction

Now fits again

Marvin-Beckmann · 2023-03-28T13:14:38Z

src/integer/vec_z/from.rs

+        assert_eq!(Z::from_i64(0), entry1);
+        assert_eq!(Z::from_i64(0), entry2);


Suggested change

assert_eq!(Z::from_i64(0), entry1);

assert_eq!(Z::from_i64(0), entry2);

assert_eq!(Z::ZERO entry1);

assert_eq!(Z::ZERO, entry2);

Marvin-Beckmann · 2023-03-28T13:15:15Z

src/integer/vec_z/from.rs

+        assert_eq!(Z::from_i64(0), entry1);
+        assert_eq!(Z::from_i64(0), entry2);


though i prefer using the constant

Suggested change

assert_eq!(Z::from_i64(0), entry1);

assert_eq!(Z::from_i64(0), entry2);

assert_eq!(Z::from(0), entry1);

assert_eq!(Z::from(0), entry2);

Changed to Z::ZERO

Marvin-Beckmann · 2023-03-28T13:37:30Z

src/integer/vec_z/transpose.rs

+        let cmp = VecZ::from_str("[[1],[2],[3]]").unwrap();
+
+        assert_eq!(cmp, vec.transpose());
+    }


Please also test with large entries and with negative entries and different sizes

several kinds of entries (large, negative etc) are tested in MatZ::transpose, which this function uses. Different sized matrices were now added (4 and 5 entries) to the ones in MatZ:: transpose by these tests

Marvin-Beckmann · 2023-03-28T13:39:42Z

src/integer/vec_z/get.rs

+    pub fn is_row_vector(&self) -> bool {
+        self.matrix.get_num_columns() > 1
+    }


would this be more intuitiv?:

Suggested change

pub fn is_row_vector(&self) -> bool {

self.matrix.get_num_columns() > 1

}

pub fn is_row_vector(&self) -> bool {

self.matrix.get_num_rows() == 1

}

Now done this way. get_direction changed appropriately

Marvin-Beckmann · 2023-03-28T13:41:08Z

src/integer/vec_z/from.rs

+    /// Creates a [`VecZ`] row-vector with entries in [`Z`] from a [`String`].
+    /// The format of that string looks like this `[1,2,3]` for a row vector
+    /// with three entries (`1` in the first row, `2` in the second one, ...)


format does not fit anymore

Should now fit, again

Marvin-Beckmann · 2023-03-28T13:44:20Z

src/integer/vec_z/from.rs

+        let entries_string = parse_matrix_string(string)?;
+        let (num_rows, num_cols) = find_matrix_dimensions(&entries_string)?;
+        let mut vector: VecZ;
+        match (num_rows, num_cols) {
+            (1, _) => vector = VecZ::new(num_cols, VectorDirection::RowVector)?,
+            (_, 1) => vector = VecZ::new(num_rows, VectorDirection::ColumnVector)?,
+            (_, _) => return Err(MathError::InvalidStringToVectorInput(String::from(
+                "The string either contained more than one column or row, or did have zero entries",
+            ))),
+        }
+
+        // fill entries of matrix according to entries in string_matrix
+        for (row_num, row) in entries_string.iter().enumerate() {
+            for (col_num, entry) in row.iter().enumerate() {
+                let z_entry = Z::from_str(entry)?;
+                vector.matrix.set_entry(row_num, col_num, z_entry)?;
+            }
+        }
+        Ok(vector)


Alternatively, what I would prefer:

Suggested change

let entries_string = parse_matrix_string(string)?;

let (num_rows, num_cols) = find_matrix_dimensions(&entries_string)?;

let mut vector: VecZ;

match (num_rows, num_cols) {

(1, _) => vector = VecZ::new(num_cols, VectorDirection::RowVector)?,

(_, 1) => vector = VecZ::new(num_rows, VectorDirection::ColumnVector)?,

(_, _) => return Err(MathError::InvalidStringToVectorInput(String::from(

"The string either contained more than one column or row, or did have zero entries",

))),

}

// fill entries of matrix according to entries in string_matrix

for (row_num, row) in entries_string.iter().enumerate() {

for (col_num, entry) in row.iter().enumerate() {

let z_entry = Z::from_str(entry)?;

vector.matrix.set_entry(row_num, col_num, z_entry)?;

}

}

Ok(vector)

let matrix = MatZ::from_str(str)?;

match (matrix.get_num_rows(), matrix.get_num_cols()) {

(a,b) if a ==1 || b== 1 => Ok(VecZ{matrix})}

_ => Err(MathError::InvalidStringToVectorInput(String::from(

"The string either contained more than one column or row, or did have zero entries",

))),

}

Done in slightly different way

Marvin-Beckmann · 2023-03-28T13:46:42Z

src/integer/vec_z.rs

+/// # Examples
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub struct VecZ {
+    pub(crate) matrix: MatZ,


matrix might be misleading

changed to vector

Marcel583 · 2023-03-28T12:59:57Z

src/utils/parse.rs

@@ -1,4 +1,4 @@
-// Copyright © 2023 Marcel Luca Schmidt
+// Copyright © 2023 Marcel Luca Schmidt, Niklas Siemer


Suggested change

// Copyright © 2023 Marcel Luca Schmidt, Niklas Siemer

// Copyright © 2023 Marcel Luca Schmidt

Marcel583 · 2023-03-28T14:00:40Z

src/integer/mat_z/transpose.rs

+    /// assert_eq!(mat.transpose(), cmp);
+    /// ```
+    pub fn transpose(&self) -> Self {
+        let mut out = Self::new(self.get_num_columns(), self.get_num_rows()).unwrap();


better use "expect" with a simple message that the number of rows or columns was saved wrongly.

As we know that this can not fail, it's ok to use unwrap

Marcel583 · 2023-03-28T14:05:00Z

src/integer/vec_z/from.rs

+    /// [`InvalidMatrix`](MathError::InvalidMatrix)
+    /// if the number of rows is `0`.
+    /// - Returns a [`MathError`] of type [`OutOfBounds`](MathError::OutOfBounds)
+    /// if the number of rows is negative or it does not fit into an [`i64`].


Suggested change

/// if the number of rows is negative or it does not fit into an [`i64`].

/// if the number of rows/columns is negative or it does not fit into an [`i64`].

Added at several points

Marcel583 · 2023-03-28T14:05:22Z

src/integer/vec_z/from.rs

+    /// # Errors and Failures
+    /// - Returns a [`MathError`] of type
+    /// [`InvalidMatrix`](MathError::InvalidMatrix)
+    /// if the number of rows is `0`.


Suggested change

/// if the number of rows is `0`.

/// if the number of rows/columns is `0`.

Marcel583 · 2023-03-28T14:09:29Z

src/integer/vec_z/from.rs

+    /// Creates a [`VecZ`] row-vector with entries in [`Z`] from a [`String`].
+    /// The format of that string looks like this `[1,2,3]` for a row vector
+    /// with three entries (`1` in the first row, `2` in the second one, ...)


Suggested change

/// Creates a [`VecZ`] row-vector with entries in [`Z`] from a [`String`].

/// The format of that string looks like this `[1,2,3]` for a row vector

/// with three entries (`1` in the first row, `2` in the second one, ...)

/// Creates a [`VecZ`] vector with entries in [`Z`] from a [`String`].

/// The format of that string looks like this `[[1,2,3]]` for a row vector

/// with three entries (`1` in the first row, `2` in the second one, ...)

/// and like this `[[1],[2],[3]]` for a column vector with the same entries.

/// A string containing a single value (`[[1]]`) is always a row vector.

Done, but left out the comment about a 1x1 string as this is the wrong place to put it.

Marcel583 · 2023-03-28T14:52:41Z

src/integer/vec_z/get.rs

+    /// assert!(row.is_row_vector());
+    /// ```
+    pub fn is_row_vector(&self) -> bool {
+        self.matrix.get_num_columns() > 1


Suggested change

self.matrix.get_num_columns() > 1

self.matrix.get_num_rows() == 1

Yes, now it's done like this and commented accordingly

Marcel583 · 2023-03-28T14:53:40Z

src/integer/vec_z/get.rs

+    /// assert!(col.is_column_vector());
+    /// ```
+    pub fn is_column_vector(&self) -> bool {
+        self.matrix.get_num_rows() > 1


Suggested change

self.matrix.get_num_rows() > 1

self.matrix.get_num_rows() != 1

Not a nice solution, the one above is way more appropriate

Marcel583 · 2023-03-28T14:53:57Z

src/integer/vec_z/get.rs

+        self.matrix.get_num_columns() > 1
+    }
+
+    /// Returns `true` if the vector is a column vector with more than one row.


Suggested change

/// Returns `true` if the vector is a column vector with more than one row.

/// Returns `true` if the vector is a column vector.

Changed according to new implementation

Marcel583 · 2023-03-28T14:54:44Z

src/integer/vec_z/get.rs

+    /// Returns [`VectorDirection::ColumnVector`] if the vector is a column vector with more than one row.
+    /// Otherwise, returns [`VectorDirection::RowVector`].


Suggested change

/// Returns [`VectorDirection::ColumnVector`] if the vector is a column vector with more than one row.

/// Otherwise, returns [`VectorDirection::RowVector`].

/// Returns [`VectorDirection::ColumnVector`] if the vector is a column vector.

/// Otherwise, returns [`VectorDirection::RowVector`].

Changed up completely

Marcel583 · 2023-03-28T15:03:06Z

src/integer/vec_z/set.rs

+    pub fn set_entry<S: TryInto<i64> + Display + Copy, T: Into<Z>>(
+        &mut self,
+        entry: S,
+        value: T,


Use updated version with impl instead of S and T

As I'm not able to use the trait and this is the way it is done in MatZ, I do not see what to do differently here. But I also opened an issue for the set_entry and get_entry trait for Vectors.

Marcel583 · 2023-03-29T11:33:07Z

src/integer/mat_z/mul.rs

+    type Output = MatZ;
+
+    /// Implements the [`Mul`] trait for two [`MatZ`] values.
+    /// [`Mul`] is implemented for any combination of owned and borrowed [`MatZ`] and any kind of [`VecZ`].


Please change

* Add macros for cross type arithmetics * Implement multiplication for MatZ * Implement PartialEq for MatZ * Implement transpose for MatZ * Add VecZ * Implement MatZ * VecZ + tests * Add instantiation methods for VecZ * Add getter and transpose to VecZ * Implement multiplication for VecZ in combination with MatZ * Apply requested PR changes and add from_str tests for VecZ * Apply rustfmt + add import of GetEntry for VecZ * Make vector viable as Column and RowVector * Apply requested PR changes to VecZ * Remove vectors

jnsiemer added 4 commits March 27, 2023 17:20

Add macros for cross type arithmetics

6f5fa12

Implement multiplication for MatZ

439aab0

Implement PartialEq for MatZ

ffb150b

Implement transpose for MatZ

39d5cdc

jnsiemer requested review from Marcel583 and Marvin-Beckmann March 27, 2023 15:29

jnsiemer self-assigned this Mar 27, 2023

jnsiemer changed the title ~~MatZ Compare + Transpose~~ MatZ Mul, Cmp, Transpose Mar 27, 2023

Marvin-Beckmann requested changes Mar 27, 2023

View reviewed changes

jnsiemer added 5 commits March 27, 2023 18:05

Add VecZ

3d7fd86

Implement MatZ * VecZ + tests

8281d0a

Add instantiation methods for VecZ

e59492f

Add getter and transpose to VecZ

9f5aa9e

Implement multiplication for VecZ in combination with MatZ

58631ab

jnsiemer force-pushed the MatZ_cmp_transpose branch from a97d74a to 58631ab Compare March 27, 2023 16:12

Apply requested PR changes and add from_str tests for VecZ

ecdd67d

jnsiemer changed the title ~~MatZ Mul, Cmp, Transpose~~ VecZ + MatZ Mul, Cmp, Transpose Mar 27, 2023

jnsiemer added 2 commits March 27, 2023 23:26

Merge branch 'dev' into MatZ_cmp_transpose

f81caef

Apply rustfmt + add import of GetEntry for VecZ

4c27d3d

jnsiemer force-pushed the MatZ_cmp_transpose branch from 1bed518 to 4c27d3d Compare March 27, 2023 21:42

jnsiemer requested a review from Marvin-Beckmann March 27, 2023 22:58

Make vector viable as Column and RowVector

b04df0d

Marvin-Beckmann requested changes Mar 28, 2023

View reviewed changes

Marcel583 requested changes Mar 28, 2023

View reviewed changes

jnsiemer mentioned this pull request Mar 28, 2023

Implement SetEntry trait for Vectors #122

Closed

Apply requested PR changes to VecZ

1681eff

jnsiemer requested review from Marcel583 and Marvin-Beckmann March 29, 2023 08:35

jnsiemer closed this Mar 29, 2023

Remove vectors

86c04e8

jnsiemer reopened this Mar 29, 2023

jnsiemer changed the title ~~VecZ + MatZ Mul, Cmp, Transpose~~ MatZ Mul, Cmp, Transpose Mar 29, 2023

Marvin-Beckmann approved these changes Mar 29, 2023

View reviewed changes

Marcel583 approved these changes Mar 29, 2023

View reviewed changes

Update doc comment to remove VecZ

be011ed

jnsiemer merged commit 1569864 into dev Mar 29, 2023

jnsiemer deleted the MatZ_cmp_transpose branch March 29, 2023 11:41

	/// [`Mul`] is implemented for any combination of owned and borrowed [`MatZ`] and any kind of [`VecZ`].
	/// [`Mul`] is implemented for any combination of owned and borrowed [`MatZ`].

		assert_eq!(Z::from_i64(0), entry1);
		assert_eq!(Z::from_i64(0), entry2);

		@@ -1,4 +1,4 @@
		// Copyright © 2023 Marcel Luca Schmidt
		// Copyright © 2023 Marcel Luca Schmidt, Niklas Siemer

	/// if the number of rows is negative or it does not fit into an [`i64`].
	/// if the number of rows/columns is negative or it does not fit into an [`i64`].

	/// if the number of rows is `0`.
	/// if the number of rows/columns is `0`.

	self.matrix.get_num_columns() > 1
	self.matrix.get_num_rows() == 1

	self.matrix.get_num_rows() > 1
	self.matrix.get_num_rows() != 1

	/// Returns `true` if the vector is a column vector with more than one row.
	/// Returns `true` if the vector is a column vector.

		/// Returns [`VectorDirection::ColumnVector`] if the vector is a column vector with more than one row.
		/// Otherwise, returns [`VectorDirection::RowVector`].

Conversation

jnsiemer commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnsiemer Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jnsiemer commented Mar 27, 2023 •

edited

Loading

jnsiemer Mar 27, 2023 •

edited

Loading