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
Unify the screen output of the different solver schemes #1983
Unify the screen output of the different solver schemes #1983
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Juliane,
Good to see you changed this. The output is in some cases already way more readable :)
What is your opinion on also including a final relative residual line for easy grepping of the final residual per time step? Or is there already an other way to easily do that or is that just an overkill of information?
source/simulator/solver_schemes.cc
Outdated
@@ -291,8 +291,9 @@ namespace aspect | |||
const double relative_stokes_residual = | |||
assemble_and_solve_stokes(nonlinear_iteration == 0, &initial_stokes_residual); | |||
|
|||
pcout << " Relative Stokes residual after nonlinear iteration " << nonlinear_iteration+1 | |||
pcout << " Nonlinear residual (Stokes system only) after nonlinear iteration " << nonlinear_iteration+1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to name them all Relative nonlinear residual
, like you suggested in #1964.
Oh, you're right, I didn't think about a way to only grep the final residual of each time step. I don't think there's a way to do that at the moment. What exactly would you want to do with the residual? Something like plotting it in gnuplot to make sure there was convergence for each time step? I guess what I am asking is: Would it make more sense to have that info in the statistics file rather than the screen output? We already have the number of iterations in the statistics file, so maybe that would be a good place to put it, and we could make that output optional? As it would be a new functionality anyway, I guess it would also make sense to put it into a separate pull request. |
Yes, I guess that you are right that it is better to put it into the statistics file since the information might not be that interesting for everyone. If you add the Relative thing, I am fine with it being merged. |
source/simulator/solver_schemes.cc
Outdated
@@ -291,8 +291,9 @@ namespace aspect | |||
const double relative_stokes_residual = | |||
assemble_and_solve_stokes(nonlinear_iteration == 0, &initial_stokes_residual); | |||
|
|||
pcout << " Relative Stokes residual after nonlinear iteration " << nonlinear_iteration+1 | |||
pcout << " Relative nonlinear residual (Stokes system only) after nonlinear iteration " << nonlinear_iteration+1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it important to differentiate between Stokes system
and Stokes system only
? The residual is calculated the same way after all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I guess it's not. I thought it would be a nice way to remind the user of the solver scheme used, but I guess that's easy to see anyway due to the equations that are solved. So should I just call it Stokes system in both cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that seems more consistent.
Apart from my one question this looks good. I agree with putting the residual into the statistics file as a separate issue. |
I opened an issue about putting the residual into the statistics file: #1986. |
13c7032
to
8e66612
Compare
Okay, I think I addressed all of your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one tiny comment left. You can merge yourself when you fixed it.
source/simulator/solver_schemes.cc
Outdated
@@ -291,8 +291,9 @@ namespace aspect | |||
const double relative_stokes_residual = | |||
assemble_and_solve_stokes(nonlinear_iteration == 0, &initial_stokes_residual); | |||
|
|||
pcout << " Relative Stokes residual after nonlinear iteration " << nonlinear_iteration+1 | |||
pcout << " Relative nonlinear residual (Stokes system) after nonlinear iteration " << nonlinear_iteration+1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one space too much between Relative and nonlinear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching that!
fdefecd
to
49dba17
Compare
This addresses #1964. The line of each nonlinear solver scheme that outputs the nonlinear residual now starts with 'Nonlinear residual ...', and then specifies the system that is being solved, following @MFraters suggestion. This should make it easier to grep that line from the screen output of different models.
In addition, there is now always one empty line between one nonliear iteration and the next, and two empty lines between the last iteration and the Postprocessing output.