Skip to content

[newchem-cpp] lookup_cool_rate1d cleanup 3/3#429

Merged
brittonsmith merged 22 commits into
grackle-project:newchem-cppfrom
mabruzzo:ncc/lookup_rate_1d_cleanup-grain-alt
Jan 13, 2026
Merged

[newchem-cpp] lookup_cool_rate1d cleanup 3/3#429
brittonsmith merged 22 commits into
grackle-project:newchem-cppfrom
mabruzzo:ncc/lookup_rate_1d_cleanup-grain-alt

Conversation

@mabruzzo

@mabruzzo mabruzzo commented Sep 26, 2025

Copy link
Copy Markdown
Collaborator

This PR shouldn't be reviewed until after #428 has been merged


Overview

This is the final PR for cleaning up lookup_cool_rate1d (there's obviously more to do, but I'm happy enough to move on)

Description

This PR leverages the GrainSpeciesInfo type introduced in #428 to refactor all of the logic pertaining to the calculation of dust grain rates.

  • Much of the work in this PR focused on replacing sections of code where we repeated identical logic for every know grain species with for-loops (of course I also tried to add in comments to explain what is going on).
  • I also excised all of the dust-logic from lookup_cool_rate1d into a helper function called lookup_dust_rate1d (this helper function is currently called by lookup_cool_rate1d, but an argument could be made that it should maybe get called separately).
  • Additionally, I significantly fleshed out the docstring for lookup_cool_rate1d

Gold Standard Considerations

Importantly, this is going to fail some of the answer tests. (After merging, we'll need to bump the gold standard).

  • This is primarily a consequence of refactoring the logic for computing the H2 formation rate on dust grains for multiple grain-species.
  • I tried multiple things, but it turns out that this was pretty unavoidable. If you look at the commit-history, you can see that I was extremely careful.
  • In fact, I confirmed that the H2 rate was only changing by a factor of ~2e-16 (it may be marginally larger than one might expect because the change affects terms in a summation).

Since we're going to bump the gold standard anyways, I took the opportunity to precompute a constant for the calculation of grain growth rates (i.e. a std::pow call and a division is now called before we "loop over a row" rather than within the loop).1 This change should definitely affect results, (to within machine tolerance), but it's not clear that it significantly shifts rates (with machine looks like this change probably doesn't change things much

Footnotes

  1. By slightly adapting the GrainSpeciesInfo machinery, it would be straightforward to precompute these constants at initialization, but we can leave that for the future.

@mabruzzo mabruzzo changed the base branch from main to newchem-cpp October 2, 2025 11:56
@mabruzzo

Copy link
Copy Markdown
Collaborator Author

@brittonsmith, conflicts have are addressed

@mabruzzo mabruzzo force-pushed the ncc/lookup_rate_1d_cleanup-grain-alt branch from 2be4289 to 6edd499 Compare December 8, 2025 18:16
@mabruzzo

mabruzzo commented Dec 8, 2025

Copy link
Copy Markdown
Collaborator Author

Ignore the force-push (I just undid an unnecessary merge with an unrelated branch)

@brittonsmith brittonsmith changed the base branch from newchem-cpp to main December 17, 2025 14:04
@brittonsmith brittonsmith changed the base branch from main to newchem-cpp December 17, 2025 14:04
@brittonsmith

Copy link
Copy Markdown
Contributor

I'm still reviewing this, but the test failures look like errors and not issues with answers. I'm seeing what looks like a segfault during one of the model tests.

Honestly, I'm not sure why it was a problem since we were copying
aggregates. In reality, my intuition is probably just wrong. Anyways, it
makes a little more sense why this error wasn't present on all
platforms.

I also introduced a test to check this kind of thing for the future
(actually, in the future this kind of test will get a lot cleaner).
@brittonsmith brittonsmith merged commit 8403af6 into grackle-project:newchem-cpp Jan 13, 2026
5 checks passed
@mabruzzo mabruzzo deleted the ncc/lookup_rate_1d_cleanup-grain-alt branch January 13, 2026 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

2 participants