-
Notifications
You must be signed in to change notification settings - Fork 1
More efficient Matrix data structure #45
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
base: brakedown
Are you sure you want to change the base?
Changes from 2 commits
04f4296
ddfd74d
8e0de99
453df5b
e20f67e
cf2af29
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,7 +54,8 @@ pub(crate) fn ceil_div(x: usize, y: usize) -> usize { | |
| pub struct Matrix<F: Field> { | ||
| pub(crate) n: usize, | ||
| pub(crate) m: usize, | ||
| entries: Vec<Vec<F>>, | ||
| row_major: Vec<Vec<F>>, | ||
| col_major: Vec<Vec<F>>, | ||
| } | ||
|
|
||
| impl<F: Field> Matrix<F> { | ||
|
|
@@ -74,32 +75,48 @@ impl<F: Field> Matrix<F> { | |
| ); | ||
|
|
||
| // TODO more efficient to run linearly? | ||
| let entries: Vec<Vec<F>> = (0..n) | ||
| let row_major: Vec<Vec<F>> = (0..n) | ||
| .map(|row| (0..m).map(|col| entry_list[m * row + col]).collect()) | ||
| .collect(); | ||
| let col_major = (0..m) | ||
| .map(|col| (0..n).map(|row| row_major[row][col]).collect()) | ||
| .collect(); | ||
|
|
||
| Self { n, m, entries } | ||
| Self { | ||
| n, | ||
| m, | ||
| row_major, | ||
| col_major, | ||
| } | ||
| } | ||
|
|
||
| /// Returns a Matrix given a list of its rows, each in turn represented as a list of field elements. | ||
| /// | ||
| /// # Panics | ||
| /// Panics if the sub-lists do not all have the same length. | ||
| pub(crate) fn new_from_rows(row_list: Vec<Vec<F>>) -> Self { | ||
| let m = row_list[0].len(); | ||
| pub(crate) fn new_from_rows(row_major: Vec<Vec<F>>) -> Self { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is only used in testing now There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can be removed / refactor tests
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that |
||
| let m = row_major[0].len(); | ||
|
|
||
| for row in row_list.iter().skip(1) { | ||
| for row in row_major.iter().skip(1) { | ||
| assert_eq!( | ||
| row.len(), | ||
| m, | ||
| "Invalid matrix construction: not all rows have the same length" | ||
| ); | ||
| } | ||
| let col_major = (0..m) | ||
| .map(|col| { | ||
| (0..row_major.len()) | ||
| .map(|row| row_major[row][col]) | ||
| .collect() | ||
| }) | ||
| .collect(); | ||
|
|
||
| Self { | ||
| n: row_list.len(), | ||
| n: row_major.len(), | ||
| m, | ||
| entries: row_list, | ||
| row_major, | ||
| col_major, | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -110,19 +127,17 @@ impl<F: Field> Matrix<F> { | |
| /// Index bound checks are waived for efficiency and behaviour under invalid indexing is undefined | ||
| #[cfg(test)] | ||
| pub(crate) fn entry(&self, i: usize, j: usize) -> F { | ||
| self.entries[i][j] | ||
| self.row_major[i][j] | ||
| } | ||
|
|
||
| /// Returns self as a list of rows | ||
| pub(crate) fn rows(&self) -> Vec<Vec<F>> { | ||
| self.entries.clone() | ||
| pub(crate) fn rows(&self) -> &Vec<Vec<F>> { | ||
| &self.row_major | ||
| } | ||
|
|
||
| /// Returns self as a list of columns | ||
| pub(crate) fn cols(&self) -> Vec<Vec<F>> { | ||
| (0..self.m) | ||
| .map(|col| (0..self.n).map(|row| self.entries[row][col]).collect()) | ||
| .collect() | ||
| pub(crate) fn cols(&self) -> &Vec<Vec<F>> { | ||
| &self.col_major | ||
| } | ||
|
|
||
| /// Returns the product v * self, where v is interpreted as a row vector. In other words, | ||
|
|
@@ -139,14 +154,7 @@ impl<F: Field> Matrix<F> { | |
| ); | ||
|
|
||
| (0..self.m) | ||
| .map(|col| { | ||
| inner_product( | ||
| v, | ||
| &(0..self.n) | ||
| .map(|row| self.entries[row][col]) | ||
| .collect::<Vec<F>>(), | ||
| ) | ||
| }) | ||
| .map(|col| inner_product(v, &self.col_major[col])) | ||
| .collect() | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is inevitable, unfortunately.
H::evaluateconsumes the vector as far as I understood. Notice that we were cloning it previously too, but it was not explicit here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But with returning a reference, now we just clone
tvectors herehttps://github.com/HungryCatsStudio/poly-commit/blob/ddfd74d7669e405d15fe5285d1fc0f6e0b5cbd92/poly-commit/src/linear_codes/mod.rs#L599-L602
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, if we mange to derive
CopyforVec<F>we can avoid cloning here too, I guessThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@autquis I figured out that you can actually just call
.borrow()and it works like a charm. See e20f67eThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mmagician Nice!
Also, let me know if I can still help with this PR!