Skip to content

[newchem-cpp] transcribe lookup_rate_1d_g#412

Merged
brittonsmith merged 57 commits into
grackle-project:newchem-cppfrom
mabruzzo:ncc/transcribe_lookup_rate_1d
Nov 12, 2025
Merged

[newchem-cpp] transcribe lookup_rate_1d_g#412
brittonsmith merged 57 commits into
grackle-project:newchem-cppfrom
mabruzzo:ncc/transcribe_lookup_rate_1d

Conversation

@mabruzzo

@mabruzzo mabruzzo commented Sep 3, 2025

Copy link
Copy Markdown
Collaborator

This PR should be reviewed after #386 and #411 are merged


This PR transcribes lookup_rate_1d_g to C++. This is pretty self-explanatory.

At the moment, this is pretty much a raw transcription (that was reformatted by clang-format). The following needs to be done before it's ready for review:

  • replace all of the internal calls that directly try to call fortran functions with calls to the nice c++ wrappers
  • clean up the signature (make it more like grackle::impl::fortran_wrapper::lookup_cool_rates1d_g)
  • remove grackle::impl::fortran_wrapper::lookup_cool_rates1d_g (and replace all calls to it with calls to grackle::impl::lookup_cool_rates1d_g). We probably also want to copy over all the notes in the docstring signature
  • fix all of the indexing (get rid of the -1 nonsense)
  • clean up some of the internal comments

Plans for followup PRs:

The C++ implementation can be cleaned up in a number of ways:

  • we should obviously replace the simple 1D interpolation for the 85 standard collisional rates to make use of for-loops that iterate over rates
  • we should factor out logic for computing different kinds of rates into different helper functions
  • we should create a nice container to track all the scratch-space (so we don't re-allocate it every time we call the function)
  • we should refactor the logic for computing k13dd to reduce the size of the temporary buffers
  • I would also really like to store the sublimation temperatures for each dust species in a table (ideally created during initialize_chemistry_data) that is indexed by the OnlyGrainSpLUT enum so we can convert all that logic to much simpler loops. (I want to use loops for all dust-related stuff, but I haven't looked to see how plausible that is)

Most (all?) of these changes are implemented in followups PRs: #415, #416, #428, #429

@mabruzzo mabruzzo added the refactor internal reorganization or code simplification with no behavior changes label Sep 3, 2025
@mabruzzo mabruzzo changed the title [newchem-cpp] WIP: transcribe lookup_rate_1d_g [newchem-cpp] transcribe lookup_rate_1d_g Sep 6, 2025
@mabruzzo mabruzzo marked this pull request as ready for review September 6, 2025 14:42
@mabruzzo mabruzzo moved this to Awaiting Review in New Chemistry and C++ Transcription Sep 6, 2025
@mabruzzo mabruzzo mentioned this pull request Sep 12, 2025
32 tasks
@mabruzzo

mabruzzo commented Oct 9, 2025

Copy link
Copy Markdown
Collaborator Author

Huh, well I just pushed a commit to fix the merge conflict and it's not showing up. I guess GitHub is struggling today

EDIT: nevermind, it all appears to have worked out

@mabruzzo mabruzzo force-pushed the ncc/transcribe_lookup_rate_1d branch from 34811ad to dfac060 Compare October 9, 2025 16:37
@brittonsmith brittonsmith merged commit c4531d3 into grackle-project:newchem-cpp Nov 12, 2025
5 checks passed
@github-project-automation github-project-automation Bot moved this from Awaiting Review to Done in New Chemistry and C++ Transcription Nov 12, 2025
@mabruzzo mabruzzo deleted the ncc/transcribe_lookup_rate_1d branch November 13, 2025 16:16
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