Skip to content

Restructure code and check inputs.#6925

Merged
gassmoeller merged 4 commits intogeodynamics:mainfrom
bangerth:restructure
Apr 7, 2026
Merged

Restructure code and check inputs.#6925
gassmoeller merged 4 commits intogeodynamics:mainfrom
bangerth:restructure

Conversation

@bangerth
Copy link
Copy Markdown
Contributor

@bangerth bangerth commented Apr 2, 2026

In trying to understand what is going on with #6924, I ran into the fact again that the dynamic_core plugin is not easy to understand. Let's refactor it as a bit to make this slightly clearer. This patch does two things:

  • There are a number of functions that compute something but return it via reference arguments. The calling sequence does not make clear what are input and which are output arguments. Make this more obvious by letting these functions return what it is they compute.
  • The plugin creates an uninitialized object at first, and only lazily initializes it later. The right thing to do is to make sure each of the functions check whether their input arguments are value.

The second commit fixes up another quirk: The plugin has many functions that compute something, but are called get_*(). We tend to think of functions that are called get_*() as ones that are "getters", i.e., functions that return the value of a variable (or perhaps return a reference). The second commit renames these functions. This commit was written by Copilot.

@Francyrad FYI

@bangerth
Copy link
Copy Markdown
Contributor Author

bangerth commented Apr 2, 2026

OK, this seems to actually work.

Copy link
Copy Markdown
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.

All of these are improvements, thank you Copilot (and Wolfgang ;-)). I have a few suggestions to make the code easier to understand, but they could also be handled in a follow-up PR. Feel free to merge this if you prefer to add them separately.

double compute_gravity_potential(const double r) const;

/**
* Calculate energy (@p Qs) and entropy (@p Es) change rate factor
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please update the documentation (which returned value is which?)

compute_specific_heating(const double Tc) const;

/**
* Calculate energy (@p Qr) and entropy (@p Er) change rate factor (regarding the
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

as above

compute_radio_heating(const double Tc) const;

/**
* Calculate energy (@p Qg) and entropy (@p Eg) change rate factor
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

and here and below for the other functions.

get_adiabatic_heating(core_data.Ti,core_data.Ek,core_data.Qk);
get_latent_heating(core_data.Ti,core_data.Ri,core_data.El,core_data.Ql);
get_heat_solution(core_data.Ti,core_data.Ri,core_data.Xi,core_data.Eh);
std::tie(core_data.Qs,core_data.Es) = compute_specific_heating(core_data.Ti);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh interesting, I didnt know about std::tie.


const double Ek = 16*numbers::PI*k_c*Utilities::fixed_power<5>(Rc)/5/Utilities::fixed_power<4>(D);
const double Qk = 8*numbers::PI*Utilities::fixed_power<3>(Rc)*k_c*Tc/Utilities::fixed_power<2>(D);
return { Ek, Qk };
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While you are changing these functions anyway, can you reorder the return values to always return Q first and E second? I spend a while checking that they are used consistently (and they are), but it would be much simpler if they just always used the same order. This only affects the functions compute_adiabatic_heating and compute_latent_heating.

Copy link
Copy Markdown
Contributor Author

@bangerth bangerth Apr 3, 2026

Choose a reason for hiding this comment

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

Good eye -- I missed that (though admittedly, I just manually wanted to make what's a return reference to a return value, with the minimal change). I asked Copilot to document the order of arguments as you asked in the other comment, and at the end it showed me this:


All five @returns statements are in place. A couple of things worth noting about the results:

  • compute_specific_heating, compute_radio_heating, and compute_gravity_heating all return (energy, entropy) — energy first.
  • compute_adiabatic_heating and compute_latent_heating return (entropy, energy) — entropy first.

This inconsistency across the five functions was already latent in the code (as noted in yesterday's review suggestions); the
new @returns lines now at least make it explicit and visible to anyone reading the header.


@bangerth
Copy link
Copy Markdown
Contributor Author

bangerth commented Apr 3, 2026

So fixed. I'm still teaching myself Copilot and what it can/cannot do. For these sorts of menial tasks, it's actually quite good, though not fast. I review all of the changes, of course, and in the end, I'm not completely sure that it is faster than doing it myself. It's interesting to experiment, though.

@Francyrad
Copy link
Copy Markdown
Contributor

So fixed. I'm still teaching myself Copilot and what it can/cannot do. For these sorts of menial tasks, it's actually quite good, though not fast. I review all of the changes, of course, and in the end, I'm not completely sure that it is faster than doing it myself. It's interesting to experiment, though.

I suggest you to use codex with GPT 5.4 extra high. Actually is the best tool usable.

@bangerth
Copy link
Copy Markdown
Contributor Author

bangerth commented Apr 3, 2026

Here, I was using Claude Sonnet 4.6 underneath Copilot. I might play with GPT 5.4 on occasion.

Copy link
Copy Markdown
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.

Thanks for addressing my comments. All good to go now.

@gassmoeller gassmoeller merged commit 827abd5 into geodynamics:main Apr 7, 2026
9 checks passed
@bangerth bangerth deleted the restructure branch April 7, 2026 21:03
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.

3 participants