Skip to content

Conversation

mabruzzo
Copy link
Collaborator

@mabruzzo mabruzzo commented Sep 2, 2025

This PR should be reviewed after #378, #401, and #402 are all merged


Importantly, this is all intended as a precursor to transcribing lookup_cool_rates1d_g.


Overview

The primary purpose of this PR is to move all of the ordinary collisional rates from within chemistry_data_storage into a single table-like data-structure.

  • the ultimate goal is to make it possible to apply a common operation on all of the 1D temperature tables in a concise, consistent manner.
    • As of this PR, we now allocate the buffers for these tables rate all at once and deallocate the tables all at once
    • In a followup PR, (immediately after we transcribe lookup_cool_rates1d_g), we will be able to convert a lot of the interpolation logic into for-loops
  • For simplicity, we organize all of this data within the CollisionalRxnRateCollection struct that previously started using within the solve_rate_cool function.
    • the choice to use this particular data structure was mostly a matter of convenience
    • I have my suspicions that we may revisit the precise choice of data structure, but that will be far less work than this initial PR (converting from struct members to a collection is far more work from converting one kind of collection to another kind).
  • The details about how exactly the data is organized is an implementation detail, I did not want these details to be exposed to Grackle users in the public headers
    • Consequently, I chose to introduce the gr_opaque_storage struct. The chemistry_data_storage struct holds an opaque pointer to the gr_opaque_storage struct, and the gr_opaque_storage struct holds the CollisionalRxnRateCollection.
    • For context, an opaque pointer is a common technique used in C APIs for hiding implementation details. The most famous example is the FILE* pointer used for IO in the standard C library (like FILE*, the definition of struct gr_opaque_storage* is never directly exposed to user-code). The docstrings for gr_opaque_storage provide additional context.
    • Strictly speaking, we could have simply embedded an opaque CollisionalRxnRateCollection pointer inside of chemistry_data_storage and avoided defining gr_opaque_storage entirely. However, I think we will gradually want to migrate more "stuff" inside of gr_opaque_storage.
  • Importantly, code outside of the grackle library can still access all of the rate data that was relocated in this PR by using the rate access API (introduced in PR [newchem-cpp] rate access api #316)

Reviewing this PR

It might be easiest to go through this commit-by-commit. All of my changes were very atomic (I tried to make the individual commits fairly descriptive)

Why this is a pre-requisite for transcribing lookup_cool_rates1d_g?

This is a prerequisite because the transcription machinery will automatically reference the rate tables in their new locations after transcription. It will save us some duplicate work.

Now that I've done all this work (it was far more involved than I expected), it has become obvious to me that the benefits to doing this work before transcribing lookup_cool_rates1d_g is actually fairly minimal. (I guess it wasn't obvious to me since its been a while since I last transcribed some routines). In any event, what's done is done

For context, the historic convention in Grackle project was to use .C as
the suffix for C++ source files. Thus, when we began transcribing files,
we used the .C suffix (even though it is a less common choice than .cpp)

However, it turns out that converting a file named `<prefix>.c` to
`<prefix>.C` causes lots of issues performing Git operations when your
machine has a case-insensitive file-system (common on macOS). This comes
up in operations as simple as changing between 2 branches

Consequently, we will be transitioning to the .cpp suffix
A few tweaks were necessary:
- I needed to add the `extern "C"` annotation to a number of functions
  that are publicly exposed as the C API
- I removed the leading underscore from some internal helper functions
- I needed to tweak make the group name argument passed to
  initialize_cloudy_data expect a `const char*`, rather than just a
  `char*`. This is essential since we are passing a string literal (this
  was a very minimal change)

I also added to the `static` specifier to some internal functions (this
ensures that these helper functions will not be accessible to external
code)
I also renamed initialize_rates.h to initialize_rates.hpp to denote that
the header is **ONLY** compatible with C++ source files
This information has been formatted into a markdown table (that will be
rendered into a table if/when we use doxygen)
These docstrings include a lot of context from the comments that were
originally in the Fortran rates calculation routines and that were
previously copied into initialize_metal_chemistry_rates.cpp (and into
initialize_rates, but that is less of a concern right now)
We plan to use `init_reaction_rate` to initialize all of the "ordinary"
collisional rates.

While I was here, I marked `add_reaction_rate` as `static` (i.e. it
isn't used outside of the translation unit corresponding to
initialize_rates.cpp)
@mabruzzo mabruzzo added the refactor internal reorganization or code simplification with no behavior changes label Sep 2, 2025
@mabruzzo mabruzzo moved this to Awaiting Review in New Chemistry and C++ Transcription Sep 2, 2025
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
Status: Awaiting Review
Development

Successfully merging this pull request may close these issues.

1 participant