Skip to content

[newchem-cpp] factor out shared logic from cool1d_multi_g and calc_tdust_3d_g#277

Merged
mabruzzo merged 12 commits into
grackle-project:newchem-cppfrom
mabruzzo:gen2024-refactor_dust_calcs
Jun 11, 2025
Merged

[newchem-cpp] factor out shared logic from cool1d_multi_g and calc_tdust_3d_g#277
mabruzzo merged 12 commits into
grackle-project:newchem-cppfrom
mabruzzo:gen2024-refactor_dust_calcs

Conversation

@mabruzzo

Copy link
Copy Markdown
Collaborator

This PR was originally proposed as brittonsmith#21


This depends on PR #276


While working on the PR (now known as) #276, I became aware that a bunch of logic (~200 lines) related to the calculation of dust temperatures was essentially duplicated between cool1d_multi_g and calc_tdust_3d_g. There were some superficial differences, but the logic very much accomplished the same thing. Thus I moved the common logic to a new Fortran subroutine called calc_all_tdust_gasgr_1d_g.1

I think this change is hugely beneficial. If we waited to refactor this until after transcribing both routines to C/C++, there was a chance that the superficial differences in the logic may have caused us to not notice the similarities.

The logic was extracted from cool1d_multi_g:

  • consequently, cool1d_multi_g should have no performance impact.
  • in contrast, calc_tdust_3d_g will probably be a little slower (we now allocate slightly more buffers than we previously did).

There is a lot of potential here to optimize the newly created calc_all_tdust_gasgr_1d_g subroutine. (We could probably reduce the number of allocated buffers to a number smaller than was originally used by calc_tdust_3d_g). But that will be easier once we have transcribed the routine (and we can use structs for organization purposes). I think this fact and the benefit of reducing duplicated code definitely makes any performance penalty of calc_tdust_3d_g a worthwhile tradeoff.

In the initial version of this PR, I manually confirmed that all tests passed.2 (I have not manually checked since updating merging in the most recent changes from newchem-cpp and porting the PR between repositories)

Footnotes

  1. When I originally wrote this PR, I had a suspicion that we could consolidate calc_all_tdust_gasgr_1d_g and calc_grain_size_increment_1d, but I'm less sure now (especially now that I have dug more deeply into the dust chemistry).

  2. Be advised, I was not extremely careful about wiring calc_grain_size_increment_1d into calc_tdust_3d_g. I kinda plopped in the new routine and addressed a few compile errors and then the tests all passed

@mabruzzo mabruzzo changed the base branch from main to newchem-cpp March 20, 2025 20:07
@mabruzzo mabruzzo moved this from In Progress to Awaiting Review in New Chemistry and C++ Transcription Mar 20, 2025
@mabruzzo mabruzzo added the refactor internal reorganization or code simplification with no behavior changes label Mar 24, 2025
@brittonsmith brittonsmith changed the base branch from newchem-cpp to main June 11, 2025 15:55
@brittonsmith brittonsmith changed the base branch from main to newchem-cpp June 11, 2025 15:55

@brittonsmith brittonsmith left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Very nice refactor. Happy for this to go in when you're ready. I realize this doesn't need #222 and #287 but I'm just going to follow my notes about PR order and play it safe.

Comment on lines +128 to +129
do i = is+1, ie+1
if ( itmask_metal(i) .ne. MASK_FALSE ) then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, do you know if it's more performant to have one do loop and itmask check and then all of the parameter-based if statements inside or multiple do/itmask loops? Maybe this is something you've thought about for the transcription. I see this is code is basically copied from its original location, so no need to change it, just curious.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Honestly, the answer is "it depends." As a general rule-of-thumb, you want to have as few branches within a for-loop as possible and generally, an if-statement corresponds to a branch (there are of course times when we can get clever and replace if-statements with a mathematical expression that doesn't have any branching).

Right now, it would definitely be faster to break this up into many for-loops. With that said, I actually think we could significantly refactor this function and get rid of a bunch of temporary buffers, which would be even better. I think it will all become a little more clear once its in C++.

@mabruzzo mabruzzo merged commit cc34ccc into grackle-project:newchem-cpp Jun 11, 2025
4 checks passed
@github-project-automation github-project-automation Bot moved this from Awaiting Review to Done in New Chemistry and C++ Transcription Jun 11, 2025
@mabruzzo mabruzzo deleted the gen2024-refactor_dust_calcs branch June 11, 2025 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor internal reorganization or code simplification with no behavior changes

Projects

Development

Successfully merging this pull request may close these issues.

2 participants