[newchem-cpp] transcribe: solve_rate_cool_g to C++#331
Conversation
this parallel clause is used when converting from comoving to proper
this parallel clause is used when converting from proper to comoving
Removed unused local variables from `calc_tdust_3d_g` that followed the naming scheme used in solve_rate_cool for tracking the mass density for the mass densities of various metal species used in dust chemistry. There was logic to convert between proper and comoving units for each of these variables, but they were not actually used for anything (which is a good thing since they were never initialized). These variable declarations were probably blindly copied from somewhere else and were never removed.
Removed unused local variables from `calc_tdust_3d_g` that followed the naming scheme used in solve_rate_cool for tracking the mass density for tracking the mass densities of various metal species used in metal chemistry. There was logic to convert between proper and comoving units for each of these variables, but they were not actually used for anything (which is a good thing since they were never initialized). These variable declarations were probably blindly copied from somewhere else and were never removed.
…arts of the code.
…cessary forward-declarations (since they already exist within grackle.h).
The goal in the transcription of solve_rate_cool_g is to avoid significant refactoring, but removing this goto statement is hugely beneficial (goto statements can invoke undefined behavior extremely easily in C++)
When I previously extracted the `step_rate_newton_raphson` subroutine out of `solve_rate_cool_g` (back when the latter was still written in Fortran), I mistakenly thought that `ierror` should be passed as an argument to `step_rate_newton_raphson` from `solve_rate_cool_g` It now became apparent to me that `ierror` is ONLY used within `step_rate_newton_raphson` as a local variable to determine control flow. As I understand it, `step_rate_newton_raphson` has a while loop and basically tries to run through the while-loop until it encounters no errors (and the answer is converged). I don't think `step_rate_newton_raphson` is able to gracefully fail (that is something to be addressed in the future). Consequently, this commit converts `ierror` from an argument of `step_rate_newton_raphson` to an internal local variable
…cribe/solve_rate_cool_g
…cribe/solve_rate_cool_g
solve_rate_cool_g to C++solve_rate_cool_g to C++
…cribe/solve_rate_cool_g
This is going to make a lot of merge conflicts
| std::memcpy(itmask_tmp, itmask, sizeof(gr_mask_type)*mask_len); | ||
| std::memcpy(itmask_nr, itmask, sizeof(gr_mask_type)*mask_len); | ||
|
|
||
| // would it be more robust to use my_chemistry->metal_cooling than imetal? |
There was a problem hiding this comment.
It can wait, but I think the answer is yes. We have discussed this before and I think we both agree that carrying a metal-related parameter and also checking for the existence of the metal density field is not something we should be doing. If the metal field is needed and not provided, we should error out.
| dtit[i] = std::fmin(dtit[i], 0.1*Heq_div_dHeqdt); | ||
| } | ||
|
|
||
| if (iter > 10) { |
There was a problem hiding this comment.
I believe the reasoning for this predates me. I genuinely wonder why we would want to do this. If I understand correctly, we could temporarily find ourselves in a state with short subcycle timesteps, get through it, and then be forced to very slowly increase the timestep again. This seems like it just makes a bad situation worse.
I'll leave this comment only to keep a record since this isn't really the place to interrogate whether this should be here.
There was a problem hiding this comment.
Just thinking out loud now, but it may be useful at a later date to understand the frequency with which various timestep limiters are coming into play and evaluating how useful they truly are.
| : MASK_FALSE; | ||
|
|
||
| // ignore metal chemistry/cooling below this metallicity | ||
| const double min_metallicity = 1.e-9 / my_chemistry->SolarMetalFractionByMass; |
There was a problem hiding this comment.
Let's not change this now since it may require a gold standard update, but I am now fairly certain it should be like this metallicity is already in units of Zsun.
| const double min_metallicity = 1.e-9 / my_chemistry->SolarMetalFractionByMass; | |
| const double min_metallicity = 1.e-9; |
| ); | ||
| } | ||
|
|
||
| // TODO: Consider refactoring the iteration mask handling: |
There was a problem hiding this comment.
In thinking about it, to me having multiple itmasks that need to coordinate is making our lives more difficult. What about a single itmask that can have more than two values. For example:
0: do nothing
1: do gauss-seidel
2: do newton-raphson v1
3: do newton-raphson v2
| // minimum elapsed time step in this row | ||
| ttmin = huge8; | ||
| for (int i = idx_range.i_start; i < idx_range.i_stop; i++) { | ||
| ttot[i] = std::fmin(ttot[i] + dtit[i], dt); |
There was a problem hiding this comment.
Maybe this is brittle or dangerous, but I wonder if all this should be inside an itmask[i] check. If we're here, there should always be at least one true itmask value still and we can save the time of looping over the whole array. This is for the future, but putting this here to make a record of it.
brittonsmith
left a comment
There was a problem hiding this comment.
This was a joy to read. I've left some notes for possible future changes, but I would rather pull this in first and do those later. I've written down all these notes locally, too. This looks great, let's take the leap.
This PR was originally proposed as brittonsmith#24
This needs to be reviewed after PRs #275, #276, #277, and #315
High-level Summary
This PR transcribes
solve_rate_cool_gfrom Fortran to C++ (the helper functions withinsolve_rate_cool_g.Fwere not transcribed). I think the following table is somewhat instructive about the significance of this PROverview
At its core, this PR is a straight-forward one-to-one from transcription from Fortran to C++. This is plainly visible in the first couple of commits. All of the subsequent commits correspond to efforts to essentially "polish" the code
All of this polishing was primarily motivated by a desire to minimize unnecessary future work and this routine's status as the lowest common ancestor of the call-graphs of every non-trivial Fortran subroutine. In more detail:
solve_rate_cool_g's arguments and hundreds of local variablesPolishing the code also made it easier to understand the general control flow (and how the routines are all inter-related), but that's honestly a secondary motivation
Important
To be clear:
More About Polishing:
enforce_max_heatcool_subcycle_dt_,select_chem_scheme_update_masks_,calc_Heq_div_dHeqdt_, &set_subcycle_dt_from_chemistry_scheme_Important new types:
IndexRange:index_helper.hfor use with transcribed routines (and general C++ compatibility) #315InternalGrUnits:code_units. I was EXTREMELY careful to make sure I preserved the original logic and make sure we used the right version of all of the constants (The value of pi differs between the C and Fortran cases and the default precision various constants differ)code_unitsin the future. For no, this simplifies a lot of transcriptionSpeciesRateSolverScratchBuf:solve_rate_cool_g(i.e. it provides a clear distinction between local variables required for cooling and variables needed for species chemistry)Places where logic changed/relocated:
gotostatement into abreakstatementierrhas been tweaked slightly. (the actual logic is all the same, but now we directly return the value)InternalGrUnitstype. But the logic has been perfectly preserved.j,kindex-pairs) was converted to use theindex_helpermachinery used in our other existing C routines (the logic is exactly equivalent and this was necessary to useInternalGrUnits)coupled_rt_modify_itmask_local function. There was a minor tweak: I moved the statement checked by both inner if-statements to the outer if-statementAssorted Closing Thoughts
The scope of this PR definitely ballooned, but I think it's worth it. All of the commits were logically ordered and have a somewhat descriptive commit message (the ordering got a little more chaotic near the end as I kept finding things that I forgot to address).
If it would make things easier, I could divide this PR into smaller parts. But you would need to be okay with merging very messy looking code into the branch.
Note
PRs for other transcribed routines should all be a lot simpler to review. As noted earlier, there were unique and strong incentives to do a lot of "polishing" in this PR.
Brief Thoughts for the future:
Footnotes
The groupings of local variables don't have to be perfect, we can always adjust them later. But, the process of aggregating them will never be easier than it is right now ↩
Additionally, there were a couple spots that had large "if-conditions" where I pre-computed parts of the conditions and temporarily stored them within local variables to simply improve legibility. The logic did not change ↩