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

Extend QN acceleration to waveform iterations #2005

Draft
wants to merge 24 commits into
base: develop
Choose a base branch
from

Conversation

NiklasKotarsky
Copy link
Collaborator

Main changes of this PR

Adds support for Quasi Newton acceleration for waveform iterations.

Motivation and additional information

Implements the algorithm described https://onlinelibrary.wiley.com/doi/10.1002/nme.6443 for fixed time grids. For moving time grids everything will be interpolated to the first time grid from the first time window inside the QNWR class instead of using the waveforms inside the QN method as done in #1834.

For IMVJ variant the same fix for all time steps as described in issue #1996 will be used.

Author's checklist

  • I used the pre-commit hook to prevent dirty commits and used pre-commit run --all to format old commits.
  • I added a changelog file with make changelog if there are user-observable changes since the last release.
  • I added a test to cover the proposed changes in our test suite.
  • For breaking changes: I documented the changes in the appropriate porting guide.
  • I sticked to C++17 features.
  • I sticked to CMake version 3.16.3.
  • 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?

@BenjaminRodenberg
Copy link
Member

@NiklasKotarsky currently this PR does not build on my system and also not in our CI pipeline. Does it build on your system? Did you maybe forget to push some file?

@BenjaminRodenberg BenjaminRodenberg added the enhancement A new feature, a new functionality of preCICE (from user perspective) label Apr 18, 2024
@NiklasKotarsky
Copy link
Collaborator Author

@NiklasKotarsky currently this PR does not build on my system and also not in our CI pipeline. Does it build on your system? Did you maybe forget to push some file?

Yes, it compiles on my system and some of the systems in the CI pipline. This is probably due to different versions of Eigen having different feature support.

@BenjaminRodenberg
Copy link
Member

@NiklasKotarsky currently this PR does not build on my system and also not in our CI pipeline. Does it build on your system? Did you maybe forget to push some file?

Yes, it compiles on my system and some of the systems in the CI pipline. This is probably due to different versions of Eigen having different feature support.

If it is an issue with different eigen version: I once had a similar problem with some features not being supported by the Eigen version used by the CI pipeline and added the following code to resolve this:

#if EIGEN_VERSION_AT_LEAST(3, 4, 0)
PRECICE_ASSERT(std::is_sorted(ts.begin(), ts.end()), "Timestamps must be sorted");
#else
PRECICE_ASSERT(std::is_sorted(ts.data(), ts.data() + ts.size()), "Timestamps must be sorted");
#endif

Which Eigen version are you using on your system?

@uekerman
Copy link
Member

uekerman commented Apr 19, 2024

For moving time grids everything will be interpolated to the first time grid from the first time window inside the QNWR class instead of using the waveforms inside the QN method as done in #1834.

Couldn't we instead use a fixed time grid with equally spaced time steps? We could hard-code this number of samples to waveform-degree + 1? Or simply to 4 for the moment.

It could also be very helpful to know how many time steps to include in W (cf. #1996) during initialization already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature, a new functionality of preCICE (from user perspective)
Projects
Waveform relaxation
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

3 participants