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

Update code style in dynamic core plugin #6177

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

gassmoeller
Copy link
Member

While reviewing #6168 I noticed that the dynamic core boundary temperature plugin could urgently use an update to our current coding style. This PR goes through the header and source file of dynamic_core and fixes a first batch of issues:

  • Added a lot of const qualifiers to variables and functions
  • Fixed whitespace between functions and made some formulas more readable
  • Improved the doxygen documentation by referencing input parameters
  • Fixed some include files (included more specifically the files that are needed instead of simulator.h)
  • Made sure declaration, definition and initialization of variables are closer together
  • replace M_PI with numbers::PI in a bunch of places

No functionality was changed and I tried to avoid changes that could introduce subtle bugs.
There is still more that could be done in a follow-up PR, but I avoided that here on purpose, e.g.:

  • Avoid handing over variables by reference to return results
  • Avoid std::pow and other unnecessary functions
  • Replace the manual (and not very accurate) heat flux computation by using the heat flux postprocessor

But I will leave that for a follow-up for later.

Copy link
Member

@tjhei tjhei left a comment

Choose a reason for hiding this comment

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

Goodness! All your changes look good. Feel free to leave the two suggestions for a followup.


/**
* Compute the core solidus at certain pressure
* Compute the core solidus at certain pressure @p pressure
* and light element concentration @p X (in wt.%).
Copy link
Member

Choose a reason for hiding this comment

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

swap the order of the parameters in the doc?

for (unsigned i=1; i<=n; ++i)
S+=B/std::sqrt(M_PI)*std::exp(-std::pow(i,2)/4.)/i*std::sinh(i*R/B);
double S=R/(2.*std::sqrt(numbers::PI));
for (unsigned int i=1; i<=n; ++i)
Copy link
Member

Choose a reason for hiding this comment

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

n is a double but compared to an int?

@gassmoeller
Copy link
Member Author

Thanks, I will open a new PR which addresses your comments and some more in a bit.

@gassmoeller gassmoeller merged commit 4cb57db into geodynamics:main Dec 11, 2024
8 checks passed
@gassmoeller gassmoeller deleted the update_dynamic_core branch December 11, 2024 12:57
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.

2 participants