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

Removes RS-LS truncation of stored columns #1257

Merged
merged 4 commits into from Aug 1, 2022
Merged

Removes RS-LS truncation of stored columns #1257

merged 4 commits into from Aug 1, 2022

Conversation

KyleDavisSA
Copy link
Contributor

@KyleDavisSA KyleDavisSA commented Apr 22, 2022

Main changes of this PR

This PR removes the arbitrary truncation of all but the first 5 columns per time window for the least squares restart for IQ-IMVJ method. This negatively impacts the performance of the RS-LS method. The accumulation of columns is handled by the filtering the of the RS-LS columns during the restart step.

Motivation and additional information

Tests have shown that this truncation ignores information helpful for convergence. The problem of too many columns used is covered by the filtering step in

impl::QRFactorization qr(_filter);
qr.setGlobalRows(getLSSystemRows());
// for QR2-filter, the QR-dec is computed in qr-applyFilter()
if (_filter != Acceleration::QR2FILTER) {
for (int i = 0; i < static_cast<int>(_matrixV_RSLS.cols()); i++) {
Eigen::VectorXd v = _matrixV_RSLS.col(i);
qr.pushBack(v); // same order as matrix V_RSLS
}
}
// apply filter
if (_filter != Acceleration::NOFILTER) {
std::vector<int> delIndices(0);
qr.applyFilter(_singularityLimit, delIndices, _matrixV_RSLS);
// start with largest index (as V,W matrices are shrinked and shifted
for (int i = delIndices.size() - 1; i >= 0; i--) {
removeMatrixColumnRSLS(delIndices[i]);
}
PRECICE_ASSERT(_matrixV_RSLS.cols() == qr.cols(), _matrixV_RSLS.cols(), qr.cols());
}

Author's checklist

  • I added a changelog file with make changelog if there are user-observable changes since the last release.
  • I ran make format to ensure everything is formatted correctly.
  • I sticked to C++14 features.
  • I sticked to CMake version 3.10.
  • I squashed / am about to squash all commits that should be seen as one.

Reviewers' checklist

  • Does the changelog entry make sense? Is it formatted correctly?
  • Do you understand the code changes?

@uekerman
Copy link
Member

Do we still need the member variable _usedColumnsPerTimeWindow then?
It's pretty suspicious that we can change sth like without any test failing. IMVJ is really not well tested.

@KyleDavisSA
Copy link
Contributor Author

No, we would not need the variable _usedColumnsPerTimeWindow. It is weird that it is there at all, and I found the results between the different restart were better by removing this.

@uekerman
Copy link
Member

We also need a changelog entry here.

@uekerman uekerman added this to the Version 2.5.0 milestone Jul 27, 2022
Copy link
Member

@uekerman uekerman left a comment

Choose a reason for hiding this comment

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

I assume that the original motivation to have this limit was efficiency of the restart, meaning that the restart itself does not become to expensive.

docs/changelog/1257.md Outdated Show resolved Hide resolved
@uekerman uekerman merged commit 6955a73 into develop Aug 1, 2022
@davidscn davidscn deleted the rslsClean branch August 1, 2022 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants