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

Expose RemotePointEvaluation parameters in non-nested multigrid #15899

Merged
merged 3 commits into from Aug 24, 2023

Conversation

fdrmrc
Copy link
Contributor

@fdrmrc fdrmrc commented Aug 20, 2023

...through an AdditionalData struct.

@fdrmrc
Copy link
Contributor Author

fdrmrc commented Aug 21, 2023

In addition to the arguments needed by the constructor of RemotePointEvaluation, I've added an additional boolean, enfroce_all_points_found, that allows to throw if not all points have been found after the search procedure. Ultimately, it calls the RPE::all_points_found().

To give more context, this PR is motivated by the fact that some boundary points may not be picked up due to floating point arithmetic in certain situations (for instance when one has neumann boundary conditions), leading to dramatic effects on the number of required iterations. This issue can be solved by simply tweaking the tolerance parameter, and this is what the present PR allows.

@fdrmrc fdrmrc force-pushed the rpe_AdditionalData_non_nested branch from 18d2575 to dda8f18 Compare August 21, 2023 12:00
Comment on lines 710 to 714
* AdditionalData structure with the arguments needed by
* RemotePointEvaluation. Default values are the same as the ones described in
* the documentation of RemotePointEvaluation. The last boolean parameter, @p enf_all_points_found is true by defaults and
* checks if RemotePointEvaluation::all_points_found() evaluates to true, i.e.
* all submitted points have been found inside the domain.
Copy link
Member

Choose a reason for hiding this comment

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

I would write the text more general, since we plan to add more parameters.

/**
* AdditionalData structure with the arguments needed by
* RemotePointEvaluation. Default values are the same as the ones described in
* the documentation of RemotePointEvaluation. The last boolean parameter, @p enf_all_points_found is true by defaults and
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* the documentation of RemotePointEvaluation. The last boolean parameter, @p enf_all_points_found is true by defaults and
* the documentation of RemotePointEvaluation. The last Boolean parameter,
* @p enf_all_points_found is true by defaults and

struct AdditionalData
{
AdditionalData(const double tol = 1e-6,
const bool enf_unique_mapping = false,
Copy link
Member

Choose a reason for hiding this comment

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

Not needed.

AdditionalData(const double tol = 1e-6,
const bool enf_unique_mapping = false,
const unsigned int rtree_l = 0,
const std::function<std::vector<bool>()> &marked_verts = {},
Copy link
Member

Choose a reason for hiding this comment

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

Not needed.

const bool enf_unique_mapping = false,
const unsigned int rtree_l = 0,
const std::function<std::vector<bool>()> &marked_verts = {},
const bool enf_all_points_found = true)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const bool enf_all_points_found = true)
const bool enforce_all_points_found = true)

Comment on lines 791 to 821
Utilities::MPI::RemotePointEvaluation<dim> rpe;
std::shared_ptr<Utilities::MPI::RemotePointEvaluation<dim>> rpe;
Copy link
Member

Choose a reason for hiding this comment

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

The change to shared_ptr is not needed!?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, it was a leftover from debugging. Fixed, thanks


if (additional_data.enforce_all_points_found)
AssertThrow(
rpe->all_points_found(),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rpe->all_points_found(),
!additional_data.enforce_all_points_found || rpe->all_points_found(),

Copy link
Member

@peterrum peterrum 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. Just some minor documentation changes.

Comment on lines 711 to 717
* @p tolerance and @p rtree_level related to the search procedure (used
* internally by RemotePointEvaluation) or, in the future, transfer operators
* needed by the non-nested multigrid algorithm. Default values are the same
* as the ones described in the documentation of RemotePointEvaluation.
* The last Boolean parameter, @p enforce_all_points_found is true by default
* and checks if RemotePointEvaluation::all_points_found() evaluates to true,
* i.e. all submitted points have been found inside the domain.
Copy link
Member

Choose a reason for hiding this comment

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

Normally, we described these parameters closer to the parameters. See

/**
* Standardized data struct to pipe additional data to the solver.
*/
struct AdditionalData
{
/**
* Constructor. By default, set the number of temporary vectors to 30,
* i.e. do a restart every 28 iterations. Also set preconditioning from
* left, the residual of the stopping criterion to the default residual,
* and re-orthogonalization only if necessary. Also, the batched mode with
* reduced functionality to track information is disabled by default.
*/
explicit AdditionalData(
const unsigned int max_n_tmp_vectors = 30,
const bool right_preconditioning = false,
const bool use_default_residual = true,
const bool force_re_orthogonalization = false,
const bool batched_mode = false,
const LinearAlgebra::OrthogonalizationStrategy
orthogonalization_strategy =
LinearAlgebra::OrthogonalizationStrategy::modified_gram_schmidt);
/**
* Maximum number of temporary vectors. This parameter controls the size
* of the Arnoldi basis, which for historical reasons is
* #max_n_tmp_vectors-2. SolverGMRES assumes that there are at least three
* temporary vectors, so this value must be greater than or equal to three.
*/
unsigned int max_n_tmp_vectors;
/**
* Flag for right preconditioning.
*
* @note Change between left and right preconditioning will also change
* the way residuals are evaluated. See the corresponding section in the
* SolverGMRES.
*/
bool right_preconditioning;
/**
* Flag for the default residual that is used to measure convergence.
*/
bool use_default_residual;
/**
* Flag to force re-orthogonalization of orthonormal basis in every step.
* If set to false, the solver automatically checks for loss of
* orthogonality every 5 iterations and enables re-orthogonalization only
* if necessary.
*/
bool force_re_orthogonalization;
/**
* Flag to control whether a reduced mode of the solver should be
* run. This is necessary when running (several) SolverGMRES instances
* involving very small and cheap linear systems where the feedback from
* all signals, eigenvalue computations, and log stream are disabled.
*/
bool batched_mode;
/**
* Strategy to orthogonalize vectors.
*/
LinearAlgebra::OrthogonalizationStrategy orthogonalization_strategy;
};
.

AssertThrow(
!additional_data.enforce_all_points_found || rpe.all_points_found(),
ExcMessage(
"You requested that all points should be found, but this didn't happen. You can change this option through the AdditionaData struct in the constructor."));
Copy link
Member

Choose a reason for hiding this comment

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

Could you break the line?

@fdrmrc fdrmrc force-pushed the rpe_AdditionalData_non_nested branch from e39c619 to f11181e Compare August 23, 2023 23:24
Copy link
Member

@peterrum peterrum left a comment

Choose a reason for hiding this comment

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

Thanks!

@peterrum peterrum merged commit 26c7c44 into dealii:master Aug 24, 2023
15 checks passed
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

2 participants