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

Use Utilities::fixed_power() in step-82. #16449

Merged
merged 1 commit into from Jan 12, 2024
Merged

Conversation

bangerth
Copy link
Member

This addresses the last uses of std::pow with fixed integer exponent. Fixes #13321.

@bangerth bangerth added this to the Release 9.6 milestone Jan 10, 2024
@marcfehling
Copy link
Member

marcfehling commented Jan 10, 2024

While there, could you also adjust these guys:

return_value = std::pow(p(0) * (1.0 - p(0)) * p(1) * (1.0 - p(1)) *
p(2) * (1.0 - p(2)),
2);

error_L2 += std::pow(u_exact.value(fe_values.quadrature_point(q)) -
solution_values_cell[q],
2) *

error_H2 += mesh3_inv *
std::pow(solution_values_neigh[q] -
solution_values[q],
2) *
dx;
error_H1 += mesh_inv *
std::pow(solution_values_neigh[q] -
solution_values[q],
2) *
dx;

@marcfehling
Copy link
Member

marcfehling commented Jan 10, 2024

After this patch, there are now ~50 occurrences of Utilities::fixed_power in step-82. I must admit that reading the code in the previous state, in particular the ExactSolution, was easier for me because there were less line breaks.

What would you think of introducing some alias in step-82, i.e.,

const auto& pow2 = Utilities::fixed_power<2>;
const auto& pow3 = Utilities::fixed_power<3>;

@bangerth
Copy link
Member Author

I addressed the remaining locations.

I am not a fan of aliases, not the least because then in the doxygen documentation generated from this program, you cannot just click on the name of the function and get to the documentation of it. I agree that the current state is not as easy to read. I don't know what to suggest otherwise, though -- I'd be ok with just dropping this patch and leaving it up to individual authors to optimize their code, and using the optimizations only in the library proper.

Copy link
Member

@marcfehling marcfehling left a comment

Choose a reason for hiding this comment

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

In terms of best practices, we should definitely use Utilities::fixed_power. I hoped there was a way to prevent line breaks to preserve readability. The manufactured solution is posted via MathJax in the introduction in a compact and readable form, so I don't feel strongly that we also need to provide it readable in the code.

@masterleinad
Copy link
Member

masterleinad commented Jan 11, 2024

I hoped there was a way to prevent line breaks to preserve readability.

You can always do

// clang-format off
/* ... */
// clang-format on

@bangerth
Copy link
Member Author

Let's not do that in tutorials, though.

@kronbichler kronbichler merged commit 5d80db2 into dealii:master Jan 12, 2024
15 checks passed
@bangerth bangerth deleted the 82 branch January 12, 2024 16:30
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.

Replace std::pow with integer exponent
4 participants