[newchem-cpp] transcribe step_rate_g#425
Merged
brittonsmith merged 12 commits intoDec 17, 2025
Merged
Conversation
mabruzzo
added
the
refactor
The transcribed function is called step_rate_gauss_seidel to mirror the naming convention of `step_rate_newton_raphson`. I explicitly disabled clang-format to make it easier for us to factor out common logic that is shared with `species_density_derivatives_0d` - In slightly more detail, the logic is similar enough that we could definitely factor out a lot of common logic. - The important thing is that we don't want the logic in each function to evolve independently
mabruzzo
force-pushed
the
ncc/transcribe_step_rate_gauss_seidel
branch
from
7ba4dbc to
586a480
Compare
mabruzzo
changed the title
step_rate_gstep_rate_g
…_step_rate_gauss_seidel
Contributor
|
@mabruzzo, just a heads-up. It looks like there is a merger conflict here now. Note sure where that came from. |
brittonsmith
approved these changes
Dec 17, 2025
| HII(i,j,k) = std::fmax((gr_float)(species_tmpdens.data[SpLUT::HII][i] ), tiny_fortran_val); | ||
| HeI(i,j,k) = std::fmax((gr_float)(species_tmpdens.data[SpLUT::HeI][i] ), tiny_fortran_val); | ||
| HeII(i,j,k) = std::fmax((gr_float)(species_tmpdens.data[SpLUT::HeII][i] ), tiny_fortran_val); | ||
| HeIII(i,j,k) = std::fmax((gr_float)(species_tmpdens.data[SpLUT::HeIII][i] ), (gr_float)(1e-5)*tiny_fortran_val); |
Contributor
There was a problem hiding this comment.
Just making a note for myself. It bugs me that we do things like this. I think it would be better if we defined the minimum value any given species is allowed to have.
Contributor
|
This looks great and is surprisingly easy to read. After looking through it, I would be open to not keeping the manual stepping for the primordial networks unless it really is a huge slowdown. |
github-project-automation
Bot
moved this to Done
in New Chemistry and C++ Transcription
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR should not be reviewed until after PR #416 has been merged
This PR attempts to transcribe
step_rate_gThe transcribed function is called
step_rate_gauss_seidelto mirror the naming convention ofstep_rate_newton_raphson.The last 2 commits of this PR split the logic of
step_rate_gauss_seidelbetween 2 helper functions (which are called bystep_rate_gauss_seidel). These helper functions are named:grackle::impl::chemistry::species_density_updates_gauss_seidel(not touched in this PR)grackle::impl::chemistry::species_density_derivatives_0dfunction)grackle::impl::chemistry::species_density_derivatives_0d(called by the newton-raphson solver) that we eventually want to refactor both functions so that the logic is shared (IMPORTANTLY, the logic in these functions should not evolve independently)grackle::impl::update_fields_from_tmpdens_gauss_seidelgrackle::impl::gauss_seidel::update_fields_from_tmpdens#434 is a followup PR that is dedicated to cleaning up this function (it also changes the function name)Aside: while clang-format is temporarily disabled for the step_rate_gauss_seidel.hpp file in this PR, it is enabled by PR #434.
I don't think the act of splitting the transcribed logic between functions will make things any harder to review. But, let me know if you would prefer that I split those 2 commits into 2 PRs.