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

Rename another function to make its intent clearer. #5314

Merged
merged 1 commit into from Jul 15, 2023

Conversation

bangerth
Copy link
Contributor

No description provided.

Comment on lines -809 to +811
void assemble_and_solve_defect_correction_Stokes(DefectCorrectionResiduals &dcr,
const bool use_picard);
void do_one_defect_correction_Stokes_step(DefectCorrectionResiduals &dcr,
const bool use_picard);
Copy link
Member

Choose a reason for hiding this comment

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

I am a bit hesitant to merge this, because all of the similar functions follow the pattern of assemble_and_solve_... for all equation systems. How do you feel about assemble_and_solve_one_defect_correction_Stokes_step?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My reasoning was that these functions appear similar, but are not in fact. The assemble_and_solve_stokes() function, for example, has 60 lines and really only does that: It assembles and solves the Stokes problem. But the function here has 180 lines, and it does a lot more: It sets up assemblers, it figures out a lot of tolerances and parameters for the nonlinear scheme, if the residual is already small it doesn't actually solve anything at all, etc. It might be useful to break an assemble_and_solve_...() function out of this one, but I don't think it actually fits the scheme of the other functions.

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

I just read through the function again and you are right the function does much more than I remembered. I am ok with the name change.

@gassmoeller gassmoeller merged commit 9ddbf3b into geodynamics:main Jul 15, 2023
6 checks passed
@bangerth bangerth deleted the rename-2 branch July 15, 2023 03:24
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