Skip to content

[newchem-cpp] Organize collisional reaction rates into an indexable table#411

Merged
mabruzzo merged 23 commits into
grackle-project:newchem-cppfrom
mabruzzo:ncc-opaque-storage
Oct 9, 2025
Merged

[newchem-cpp] Organize collisional reaction rates into an indexable table#411
mabruzzo merged 23 commits into
grackle-project:newchem-cppfrom
mabruzzo:ncc-opaque-storage

Conversation

@mabruzzo

@mabruzzo mabruzzo commented Sep 2, 2025

Copy link
Copy Markdown
Collaborator

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

@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
@brittonsmith brittonsmith changed the base branch from newchem-cpp to main October 6, 2025 15:39
@brittonsmith brittonsmith changed the base branch from main to newchem-cpp October 6, 2025 15:39

@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.

This looks like a good refactor to me. I mostly had questions that I am fairly certain you've already thought about. Feel free to merge this and I can have a look at any responses you have after.

Comment thread src/clib/initialize_rates.cpp Outdated
// , my_rates->k125[i]
// , my_rates->k129[i]
// , my_rates->k130[i]);
kcol_rate_tables->data[CollisionalRxnLUT::kz15][i] = 4.98e-11;

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.

You mentioned not being a big fan of the add_reaction_rate strategy of defining individual functions for each rate. Do you foresee this getting refactored in another way to unify it with how the rest of the reaction rates are initialized? I actually like the individual rate function strategy for the transparency and ability to provide specific, detailed documentation. Either way, I'm curious where you see this going.

Comment on lines +276 to +278
// TODO: in the future, we should refactor to make this logic look more
// consistent with the logic for other similar rates in
// initialize_rates

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.

Maybe this answers my previous question about standardizing with the other rates.

double units,
chemistry_data *my_chemistry)
{
*rate_ptr = (double*)malloc(my_chemistry->NumberOfTemperatureBins

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.

Is it worth switching to new at this time?

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.

I looked into it and in this particular case, it's not worth swapping to new (in the context of the current PR). It turns out that this malloc is being used for allocating all of the species-dependent cooling rates that I didn't touch in this PR. (Consequently, it would involve touching a bunch of new code).

With that said, I change the malloc of gr_opaque_storage to a new and the malloc for the member of gr_opaque_storage to a new

In particular I:
- reordered initialization rates
- switched from using `malloc` to using `new` for `gr_opaque_storage`
  and its imediate contents
@mabruzzo mabruzzo force-pushed the ncc-opaque-storage branch from d4564fc to ee2822f Compare October 9, 2025 14:04
@mabruzzo mabruzzo merged commit af2447c into grackle-project:newchem-cpp Oct 9, 2025
5 checks passed
@mabruzzo mabruzzo deleted the ncc-opaque-storage branch October 9, 2025 15:12
@github-project-automation github-project-automation Bot moved this from Awaiting Review to Done in New Chemistry and C++ Transcription Oct 9, 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

Development

Successfully merging this pull request may close these issues.

2 participants