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

Cleanup of SolverGMRES and SolverFGMRES implementations #16745

Merged
merged 3 commits into from Mar 15, 2024

Conversation

kronbichler
Copy link
Member

This PR does three major cleanup steps in the SolverGMRES and SolverFGMRES implementation:

  • Make SolverFGMRES also use the Givens rotations for the orthogonalization, rather than the Householder transformation. (This changes a few tests to slightly lower iterations, I checked that the real residual is correct.) This is achieved by making the Givens rotation a free function.
  • Use more similar variable names in both solvers to more easily understand them both. In particular, choose inner_iteration as the name for the inner iteration, rather than dim (mixed into it) or j.
  • Make the size of the basis the default of SolverGMRES::AdditionalData. We had a todo by Guido for a long time, and I think it is time to make this change now. This also helps the similarity between the classes. Note that this can affect tests, where I opted for changing the parameter where necessary.

If this finds approval, I will write a changelog.

@peterrum
Copy link
Member

If this finds approval, I will write a changelog.

I am in favor 👍

@kronbichler kronbichler force-pushed the gmres_cleanup branch 3 times, most recently from dece2d4 to d097676 Compare March 12, 2024 06:57
@kronbichler
Copy link
Member Author

I now added two changelog entries, one describing the incompatible choice for the size of the Arnoldi basis and one for the algorithmic change in the SolverFGMRES. Regarding the former, I do not see another way than letting the new max_basis_size take precedence, even though I anticipate that some users might set additional_data.max_n_tmp_vectors = xx; in a separate statement, that would now be invalidated by the value of max_basis_size in the constructor. Otherwise, I would have to guess what the user intended, e.g. letting max_n_tmp_vectors take precedence whenever max_basis_size is set to 30, but how can I know that this comes from the default constructor of SolverGMRES::AdditionalData than something the user wants to happen? Thus, let us simply make the incompatible change now.

* temporary vectors, so this value must be greater than or equal to three.
* Maximum number of temporary vectors. Together with #max_basis_size,
* this parameter controls the size of the Arnoldi basis, which for
* historical reasons is #max_n_tmp_vectors-2. SolverGMRES assumes that
Copy link
Member

Choose a reason for hiding this comment

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

The usage of # is the first I see it. Do we need it? SolverGMRES does not and still creates a link in Doxygen.

Copy link
Member Author

Choose a reason for hiding this comment

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

I copied it from somewhere else in this file and then just went with it by routine; thinking about it I will remove it, thanks.

@kronbichler
Copy link
Member Author

Any additional comments on this PR? I would like to finalize this one soon, because #16749 sits on top of this branch and things get messy if they diverge too much (there is already some tiny deviation).

Copy link
Member

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Looks good to me. I would maybe also be fine with removing max_n_tmp_vectors already since we can't deprecate it properly anyway.

@kronbichler kronbichler merged commit 3ccc91f into dealii:master Mar 15, 2024
16 checks passed
@kronbichler kronbichler deleted the gmres_cleanup branch March 15, 2024 06:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants