Skip to content

Conversation

@mabruzzo
Copy link
Collaborator

@mabruzzo mabruzzo commented Jan 1, 2026

To be reviewed after #479 is merged


This PR replaces all hard-coded injection pathway data-members from the grackle_field_data (e.g. grackle_field_data::local_ISM_metal_density, grackle_field_data::y19_metal_density, ...) with an array of pointers known as grackle_field_data::inject_pathway_metal_density. In other words, Grackle no longer hardcodes the names of each injection pathway members as part of the grackle_field_data struct. (To determine the names, users must use the query-machinery introduced in #479).

This is significant for 2 reasons:

  1. I think the old approach would have been a pretty bad choice from an API perspective
    • injection pathway models or supernova models is the exact sort of thing where one might expect "churn"
    • under the old approach, we would have needed to keep adding more and more members to grackle_field_data whenever someone wanted another model (while maintaining members for all existing models)
    • Likewise, a simulation code that wanted to use another model would need to add several lines of code to support each kind of model
    • the newer approach is much more scalable
  2. It's also important from an implementation perspective:
    • the number of inject field paths are now entirely determined by the values read in from the table. To add/remove/rename pathways, you "just" need to modify inject_model/raw_data.[ch]pp and inject_model/load_data.[ch]pp.
    • this is a massive step that will make it possible to move this data into HDF5 tables (and let users supply arbitrary data)

There is some unforunate consequences, but if we don't do this you end
up with some pretty confusing looking code... (the confusion primarily
arises if new_FrozenKeyIdxBiMap or drop_FrozenKeyIdxBiMap has different
behavior from other functions with similar names). I really think it
would be better to make this into a simple C++ class with a constructor
and destructor, but thats a topic for another time
There's still a lot of work to be done (the type is incomplete and isn't
filled by anything at all)
We don't yet use the new format, but we are just about ready to start
doing so
@mabruzzo mabruzzo added the refactor internal reorganization or code simplification with no behavior changes label Jan 1, 2026
@mabruzzo mabruzzo marked this pull request as draft January 1, 2026 21:46
@mabruzzo mabruzzo force-pushed the ncc/remove-InjectPathFieldPack branch from 1b615a0 to 1bbb83e Compare January 3, 2026 15:10
@mabruzzo mabruzzo force-pushed the ncc/remove-InjectPathFieldPack branch 3 times, most recently from 643a69c to a41f096 Compare January 3, 2026 21:18
@mabruzzo mabruzzo force-pushed the ncc/remove-InjectPathFieldPack branch from a41f096 to 7e2d847 Compare January 4, 2026 00:35
This is uglier, but the number of inject field paths are now entirely
determined by the values read in from the table. To add/remove/rename
pathways, you "just" need to modify `inject_model/raw_data.[ch]pp` and
`inject_model/load_data.[ch]pp`.

This is a massive step that will make it possible to move this data into
HDF5 tables
@mabruzzo mabruzzo changed the title WIP: [newchem-cpp] Remove Hard-coded injection path names from grackle_field_data [newchem-cpp] Remove Hard-coded injection path names from grackle_field_data Jan 4, 2026
@mabruzzo mabruzzo marked this pull request as ready for review January 4, 2026 04:02
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: No status

Development

Successfully merging this pull request may close these issues.

1 participant