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

Clarify function argument names. #5291

Merged
merged 2 commits into from Jul 14, 2023
Merged

Conversation

bangerth
Copy link
Contributor

We have three functions that can optionally compute something. Whether they do is controlled by a bool argument, and the output is put into a pointer variable. This is duplicative: We should just use whether the pointer is non-nullptr to control whether or not to compute the optional thing.

@gassmoeller
Copy link
Member

I am not sure the bool is duplicative. This function can be called in three ways:

  • In a 'single' non iterative solver scheme no pointer is provided and none should be computed
  • In iterative schemes the functions is called in two different ways:
    • a pointer is provided and should be filled
    • a pointer is provided but should not be filled, instead it is used to compute the relative residual that is returned

There may be better ways to indicate which operation is requested, but I think a single pointer is probably not enough (unless we use *pointer == 0.0 as a condition to indicate the second case above, but that is fragile, because the residual might just be zero because we are converged.

@bangerth
Copy link
Contributor Author

@gassmoeller Oh, I should have checked here. I think you also figured out what I describe in #5300.

@bangerth bangerth force-pushed the remove branch 2 times, most recently from 5799b0e to f9020a7 Compare July 14, 2023 05:06
@bangerth
Copy link
Contributor Author

Let's see if this actually works finally. I think that I finally understood how this is supposed to work; perhaps I have managed to even express my understanding in code. The PR was initially called "Remove a function argument", which I've come to believe is not actually possible, but perhaps the clarification of the argument names is still useful.

@bangerth bangerth changed the title Remove a function argument. Clarify function argument names. Jul 14, 2023
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 am not sure I find the new version easier to understand than the old one, but it is not worse either. If you address my comments this is ok to merge.

@@ -993,10 +987,12 @@ namespace aspect
particle_world->restore_particles();

const double relative_temperature_residual =
assemble_and_solve_temperature(nonlinear_iteration == 0, &initial_temperature_residual);
assemble_and_solve_temperature(initial_temperature_residual,
nonlinear_iteration == 0 ? &initial_temperature_residual : 0);
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
nonlinear_iteration == 0 ? &initial_temperature_residual : 0);
nonlinear_iteration == 0 ? &initial_temperature_residual : nullptr);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!


const std::vector<double> relative_composition_residual =
assemble_and_solve_composition(nonlinear_iteration == 0, &initial_composition_residual);
assemble_and_solve_composition(initial_composition_residual,
nonlinear_iteration == 0 ? &initial_composition_residual : 0);
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
nonlinear_iteration == 0 ? &initial_composition_residual : 0);
nonlinear_iteration == 0 ? &initial_composition_residual : nullptr);

@bangerth
Copy link
Contributor Author

Yes, I ended up going around a bit with this PR and ended up in a not vastly different place. Perhaps the biggest difference is to rename the variables. My confusion, as stated in #5300, was mainly that the variable was called "initial_residual", but that it is computed based on a boolean argument that at least in this function we don't know is only ever true the first time around. It looks like we're putting the current residual into the initial_residual variable, and that just seems wrong. I think that the current variable naming is easier to read, though the patch does not otherwise vastly improve over what was there before.

@gassmoeller gassmoeller merged commit 57653ee into geodynamics:main Jul 14, 2023
6 checks passed
@bangerth bangerth deleted the remove branch July 14, 2023 21:09
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