-
Notifications
You must be signed in to change notification settings - Fork 66
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
[Persistence matrix] Better additions in U for RU matrices in Z2 #1143
Changes from all commits
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 |
---|---|---|
|
@@ -615,7 +615,7 @@ inline void Base_matrix<Master_matrix>::print() | |
if constexpr (Master_matrix::Option_list::has_row_access) { | ||
std::cout << "Row Matrix:\n"; | ||
for (Index i = 0; i < nextInsertIndex_; ++i) { | ||
const auto& row = RA_opt::rows_[i]; | ||
const auto& row = (*RA_opt::rows_)[i]; | ||
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. Just to confirm: this is a bugfix independent of the main goal of the PR, right? 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. Yes, I stumbled over it while testing the new strategy. |
||
for (const auto& entry : row) { | ||
std::cout << entry.get_column_index() << " "; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -538,7 +538,7 @@ inline void RU_matrix<Master_matrix>::insert_boundary(ID_index cellIndex, | |
if constexpr (Master_matrix::Option_list::has_vine_update) { | ||
if (cellIndex != nextEventIndex_) { | ||
Swap_opt::_positionToRowIdx().emplace(nextEventIndex_, cellIndex); | ||
if (Master_matrix::Option_list::has_column_pairings) { | ||
if constexpr (Master_matrix::Option_list::has_column_pairings) { | ||
Swap_opt::template RU_pairing<Master_matrix>::idToPosition_.emplace(cellIndex, nextEventIndex_); | ||
} | ||
} | ||
|
@@ -638,7 +638,7 @@ inline void RU_matrix<Master_matrix>::add_to(Index sourceColumnIndex, Index targ | |
{ | ||
reducedMatrixR_.add_to(sourceColumnIndex, targetColumnIndex); | ||
// U transposed to avoid row operations | ||
if constexpr (Master_matrix::Option_list::has_vine_update) | ||
if constexpr (Master_matrix::Option_list::is_z2) | ||
mirrorMatrixU_.add_to(targetColumnIndex, sourceColumnIndex); | ||
else | ||
mirrorMatrixU_.add_to(sourceColumnIndex, targetColumnIndex); | ||
|
@@ -649,6 +649,8 @@ inline void RU_matrix<Master_matrix>::multiply_target_and_add_to(Index sourceCol | |
const Field_element& coefficient, | ||
Index targetColumnIndex) | ||
{ | ||
static_assert(!Master_matrix::Option_list::is_z2, | ||
"Multiplication with something else than the identity is not allowed with Z2 coefficients."); | ||
Comment on lines
+652
to
+653
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. How do you know that the argument is not the identity? 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. I don't know, I just wanted to say that for Z2 |
||
reducedMatrixR_.multiply_target_and_add_to(sourceColumnIndex, coefficient, targetColumnIndex); | ||
mirrorMatrixU_.multiply_target_and_add_to(sourceColumnIndex, coefficient, targetColumnIndex); | ||
} | ||
|
@@ -658,6 +660,8 @@ inline void RU_matrix<Master_matrix>::multiply_source_and_add_to(const Field_ele | |
Index sourceColumnIndex, | ||
Index targetColumnIndex) | ||
{ | ||
static_assert(!Master_matrix::Option_list::is_z2, | ||
"Multiplication with something else than the identity is not allowed with Z2 coefficients."); | ||
reducedMatrixR_.multiply_source_and_add_to(coefficient, sourceColumnIndex, targetColumnIndex); | ||
mirrorMatrixU_.multiply_source_and_add_to(coefficient, sourceColumnIndex, targetColumnIndex); | ||
} | ||
|
@@ -844,19 +848,17 @@ inline void RU_matrix<Master_matrix>::_reduce_column_by(Index target, Index sour | |
curr += reducedMatrixR_.get_column(source); | ||
// to avoid having to do line operations during vineyards, U is transposed | ||
// TODO: explain this somewhere in the documentation... | ||
if constexpr (Master_matrix::Option_list::has_vine_update) | ||
mirrorMatrixU_.get_column(source) += mirrorMatrixU_.get_column(target); | ||
else | ||
mirrorMatrixU_.get_column(target) += mirrorMatrixU_.get_column(source); | ||
mirrorMatrixU_.get_column(source).push_back(*mirrorMatrixU_.get_column(target).begin()); | ||
} else { | ||
Column& toadd = reducedMatrixR_.get_column(source); | ||
Field_element coef = toadd.get_pivot_value(); | ||
coef = operators_->get_inverse(coef); | ||
operators_->multiply_inplace(coef, operators_->get_characteristic() - curr.get_pivot_value()); | ||
|
||
curr.multiply_source_and_add(toadd, coef); | ||
// but no transposition for Zp, careful if there will be vineyard or rep cycles in Zp one day | ||
// TODO: explain this somewhere in the documentation... | ||
mirrorMatrixU_.multiply_source_and_add_to(coef, source, target); | ||
// mirrorMatrixU_.get_column(target).multiply_source_and_add(mirrorMatrixU_.get_column(source), coef); | ||
} | ||
} | ||
|
||
|
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.
I think I would have preferred a name like
insert_pivot
orinsert_max_entry
, but this is ok for now.