transcribe lookup_cool_rates0d 2/2#33
Closed
mabruzzo wants to merge 691 commits into
Closed
Conversation
This was referenced Feb 20, 2025
…te job to the corelib-tests
Originally one job ran the tests after building pygrackle as a standalone package (i.e. the core Grackle is compiled as part of pygrackle). The other job ran the tests with versions of pygrackle where we explicitly built the core library (with both build-systems) as a separate step from building pygrackle.
In these cases, we only run a subset of the test-suite (the much faster component). This seems reasonable since we are already running the answer-tests with the version of pygrackle configured that automatically compiled the core-library as part of the build process. This should save a lot of time
Optimize circleci workflows
remove 2 unneeded files
…sting-without-editable-install Support running subset of pygrackle test without a an editable install
remove a line that was made obsolete by PR grackle-project#224
…tweak cython does not need to be a runtime-dependency
…nstruction-tweaks Pygrackle test instruction tweaks
fix a typo in a CMake variable name
Tweak cython-handling in build-system
…Warnings Enable Compiler Warnings in a CI job
…lding_length_fix H2 self shielding length scale factor fix
…ranscribe/Implement-SpLUT-with-XMacros
…n2024transcribe/SpeciesLUTFieldAdaptor
…ranscribe/initial-step_rate_newton_raphson
…o gen2024transcribe/step_rate_newton_raphson-cleanup-1of2
…e/internal_types_updates [newchem-cpp] internal scratch-buffer interface update
…e/Implement-SpLUT-with-XMacros [newchem-cpp] Refactor such that SpLUT is implemented with XMacros
…e/SpeciesLUTFieldAdaptor [newchem-cpp] Introduce `SpeciesLutFieldAdaptor`
The *Great Renaming:* `gracklepy` edition
…e/initial-step_rate_newton_raphson [newchem-cpp] initial transcription of `step_rate_newton_raphson`
…phson-cleanup-1of2
…phson-cleanup-1of2
…' into gen2024transcribe/lookup_cool_rates0d
…scribe/lookup_cool_rates0d_cleanup
Author
|
superseded by grackle-project#379 |
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
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 must be reviewed after PR #31
This PR finishes the transcription of
lookup_cool_rates0dthat I started back in #30This PR can be broadly summarized as the following:
lookup_cool_rated0dinto a function calledspecies_density_derivatives_0dlookup_cool_rates0d1/2 #31 (and in the functions docstring), Gen seems to have copied a lot of logic fromstep_rate_gand then made a series of systematic changes. After we transcribestep_rate_g, I have some thoughts on how to reconcile the 2 implementationchemistry_solver_funcs.hppfile. The idea is to eventually store related logic fromlookup_cool_rates1d_g,step_rate_gandrate_timestep_gwithin this file. The goal is to try to store chemistry-logic called here from the grackle solver. To the extent that it's possible, it would be nice to decouple this logic from the solver's underlying scheme (whether it's the classic partial updates with the backward-difference-formula OR newton-raphson OR something else)lookup_cool_rates0dandtime_deriv_0d::derivatives. The fused function is calledtime_deriv_0d::derivatives. This has been my intention for a while. The name now much more clearly conveys the purpose of the function (while it invokeslookup_cool_rates1d, the purpose of the function is much different).time_deriv_0d::derivatives. This involved adding an ugly wrapper function that gets called bystep_rate_newton_raphson. That is a short-term hack. My next intention is to finish cleaning upstep_newton_raphson(I simply wanted to minimize the amount I changed that function in this PR)Overall, I think the end result of this PR is a massive improvement over the original fortran routine (and it has much better documentation). While improvements are still possible,1 I think we're done with transcribing this function.
Future Thoughts
It also would make a lot of sense to refactor the function so that it can compute derivatives for multiple sets of conditions at once (i.e. so it's no longer 0d). Alternatively, it might more sense to more directly compute partial derivatives like$\frac{\partial }{\partial \rho_i} \dot{\rho}_i$ or $\frac{\partial }{\partial T} \dot{\rho}_i$ . With a little refactoring that I want to do anyways, these calculations would be trivial
Misc
I have manually confirmed that all tests pass
Footnotes
For example, now that I can see the full picture, I would make some different organizational choices. It also would make a lot of sense to refactor the function so that it can compute derivatives for multiple sets of conditions at once (i.e. so it's no longer 0d). Alternatively, it would might more sense to more directly compute $
\frac{\partial}{\partial \rho_i$ partial derivatives. With a little refactoring that I want to do anyways, this would become ↩