Refactored line search functionality for do_one_defect_correction_Stokes_step in solver_schemes.cc #6772

arnoldkk13 · 2025-11-11T17:30:04Z

Pull Request Checklist. Please read and check each box with an X. Delete any part not applicable. Ask on the forum if you need help with any step.

Refactored the Newton line search functionality in do_one_defect_correction_Stokes_step into a new function in helper_functions.cc.

Before your first pull request:

[X ] I have read the guidelines in our CONTRIBUTING.md document.

For all pull requests:

[X ] I have followed the instructions for indenting my code.

For new features/models or changes of existing features:

[X ] I have tested my new feature locally to ensure it is correct.

include/aspect/simulator.h

source/simulator/helper_functions.cc

include/aspect/simulator.h

bangerth · 2025-11-11T21:10:32Z

source/simulator/helper_functions.cc

+   * Performs a Newton line search for the do_one_defect_correction_Stokes_step function
+   */
+  template <int dim>
+  void Simulator<dim>::perform_line_search(const DefectCorrectionResiduals &dcr, const bool use_picard, LinearAlgebra::BlockVector &search_direction)


same here:

return step length

one argument per line

bangerth · 2025-11-11T21:20:00Z

include/aspect/simulator.h


+      /**
+       * Perform a newton line search to determine the optimal step length
+       * along the search direction.


Also say that the result of the function is that current_linearization_point is set to the old current_linearization_point plus a suitable update in direction of search_direction.

You'll still want to address this point.

Also, document what the function returns.

bangerth

Only the two comments I'm leaving below.

bangerth · 2025-11-14T22:51:04Z

include/aspect/simulator.h


+      /**
+       * Perform a newton line search to determine the optimal step length
+       * along the search direction.


You'll still want to address this point.

bangerth · 2025-11-14T22:54:03Z

include/aspect/simulator.h


+      /**
+       * Perform a newton line search to determine the optimal step length
+       * along the search direction.


Also, document what the function returns.

bangerth

This looks good to me. @arnoldkk13 is one of my undergraduate research students, so I would like someone else to take a second look and approve/disapprove.

We have some follow-ups in the pipeline. For example, the search_direction argument to the refactored function should really be const, but the original code modified it so the current code does too. We'll work on that next, once this patch is in.

gassmoeller

Looks all correct to me, I just have some code style suggestion, and a question about a change in the screen output. Let me know when you want me to take another look, and thanks @arnoldkk13!

gassmoeller · 2025-11-21T13:24:33Z

include/aspect/simulator.h

+      * is set to the old current_linearlization_point plus a suitable update.
+      *
+      * @return This function returns the updated residual after the line
+      * search is performed.


Could you indent this comment like the other lines? Unfortunately make indent does not affect doxygen comments so you will have to do it manually.

Also please add documentation for the other input parameters before @return in this format: * @param use_picard Whether a Picard iteration was used to ... and the same for the other parameters.

gassmoeller · 2025-11-21T13:30:17Z

source/simulator/helper_functions.cc

+  /**
+   * Performs a Newton line search for the do_one_defect_correction_Stokes_step function
+   */


Remove this comment here. The place to put these comments is the declaration of this function in the header file. And please add two more empty lines, we have a stylistic convention of having 3 empty lines between functions in all source files, and following this convention makes it easier to read the code.

gassmoeller · 2025-11-21T13:33:49Z

source/simulator/helper_functions.cc

+    // Do the loop for the line search at least once with the full step length.
+    // If line search is disabled we will exit the loop in the first iteration.
+


I think the code would be easier to read if you had an empty line before the comment and no empty line between comment and the loop. It is just a quesiton of coding style, but code is typically read much more often than (re-)written and this is closer to our usual style.

source/simulator/helper_functions.cc

gassmoeller · 2025-11-21T13:37:45Z

source/simulator/solver_schemes.cc

-                pcout << "   The linear solver tolerance is set to " << parameters.linear_stokes_solver_tolerance << std::endl;
+                pcout << "   The linear solver tolerance is set to "
+                      << parameters.linear_stokes_solver_tolerance
+                      << ". ";


You removed an std::endl at the end of this line. Was this on purpose? If it was: Can you post how the output looked like before and now? I just want to check if it makes sense to remove the newline in this context.

arnoldkk13 · 2025-11-21T22:56:50Z

@gassmoeller Thank you for the comments and changes! I’ve finished adding your comments, so please take a look when you have the chance.

bangerth · 2025-11-23T09:51:43Z

include/aspect/simulator.h

+      /**
+       * Perform a Newton line search to determine the optimal step length
+       * along the search direction. After the update, the current_linearization_point
+       * is set to the old current_linearlization_point plus a suitable update.
+       *
+      * @param dcr The defect correction residuals associated with the current nonlinear
+      * iteration.
+      * @param use_picard Whether a Picard iteration was used to update the nonlinear
+      * iteration (true) or a Newton update (false).
+      * @param search_direction The proposed update direction for the solution vector.
+      *
+       * @return This function returns the updated residual after the line
+       * search is performed.
+       *
+       * This function is implemented in
+       * <code>source/simulator/helper_functions.cc</code>
+       */


You'll still want to align the stars at the beginning of each line.

gassmoeller

Yes, looks all good now. Thanks!

gassmoeller · 2025-11-28T15:24:27Z

Ah this PR has acquired a conflict because I merged #6782. Could you update your main branch and then rebase this PR to resolve the conflict? Let us know if you need instructions for how that works.

bangerth · 2025-11-30T13:25:05Z

@arnoldkk13 Let me know if you need help with resolving the merge conflict!

arnoldkk13 · 2025-12-05T00:07:03Z

I believe that the merge conflict should be resolved. Please let me know if any changes are needed!

bangerth added the ready to test label Nov 11, 2025

bangerth reviewed Nov 11, 2025

View reviewed changes

arnoldkk13 force-pushed the defect_correction_stokes_refactor branch from 8a53156 to 90b1500 Compare November 14, 2025 00:57

bangerth reviewed Nov 14, 2025

View reviewed changes

arnoldkk13 force-pushed the defect_correction_stokes_refactor branch from 0de2e51 to c537189 Compare November 18, 2025 05:19

bangerth reviewed Nov 18, 2025

View reviewed changes

gassmoeller reviewed Nov 21, 2025

View reviewed changes

arnoldkk13 force-pushed the defect_correction_stokes_refactor branch from 12ce145 to c637833 Compare November 21, 2025 22:53

bangerth reviewed Nov 23, 2025

View reviewed changes

arnoldkk13 force-pushed the defect_correction_stokes_refactor branch from 0604575 to 0528889 Compare November 23, 2025 19:52

gassmoeller approved these changes Nov 28, 2025

View reviewed changes

Refactor the Newton line search.

679066c

arnoldkk13 force-pushed the defect_correction_stokes_refactor branch from 9416939 to 679066c Compare December 5, 2025 00:05

		// Do the loop for the line search at least once with the full step length.
		// If line search is disabled we will exit the loop in the first iteration.

Refactored line search functionality for do_one_defect_correction_Stokes_step in solver_schemes.cc #6772

Are you sure you want to change the base?

Refactored line search functionality for do_one_defect_correction_Stokes_step in solver_schemes.cc #6772

Uh oh!

Conversation

arnoldkk13 commented Nov 11, 2025

Before your first pull request:

For all pull requests:

For new features/models or changes of existing features:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bangerth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bangerth left a comment

Choose a reason for hiding this comment

Uh oh!

gassmoeller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnoldkk13 commented Nov 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gassmoeller left a comment

Choose a reason for hiding this comment

Uh oh!

gassmoeller commented Nov 28, 2025

Uh oh!

bangerth commented Nov 30, 2025

Uh oh!

arnoldkk13 commented Dec 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants