Skip to content

Transcribe interpolate routines#38

Closed
mabruzzo wants to merge 714 commits into
brittonsmith:gen2024from
mabruzzo:gen2024transcribe/interpolators
Closed

Transcribe interpolate routines#38
mabruzzo wants to merge 714 commits into
brittonsmith:gen2024from
mabruzzo:gen2024transcribe/interpolators

Conversation

@mabruzzo

Copy link
Copy Markdown

This should be reviewed after PR #37 is merged


This PR introduces C++ transcriptions of the interpolate subroutines and a series of rigorous tests for each routine (the tests show that the transcribed routines produce bitwise identical results to the original versions)

This PR is a little unique among transcription PRs. Specifically, it has 2 properties that I would ordinarily consider to be grounds for rejecting a "transcription PR." However, this PR is somewhat special and we have strong justifications for both cases:

  1. it introduces the transcribed code (for multiple routines) in a nearly finished state in a single commit
    • Context: Having a single commit where a lot of logic is transcribed and a lot of "transformations" are performed on the transcribed is problematic for 2 reasons: (i) the PR is harder to review and (ii) if we later detect buggy behavior, the behavior's origin is much harder to detect1
    • Justification for this PR:
      1. This PR is the direct successor to my Transcribe contents of interpolators_g.F from Fortran to C grackle-project/grackle#160 and Modern Version Of: Transcribe contents of interpolators_g.F from Fortran to C grackle-project/grackle#251 PRs. Those PRs manually introduced the transcribed versions of the logic in a very logical commit-by-commit manner. It would be a significant amount of work to retain that detailed commit-history (given how much the rest of the code has changed), so I decided to not to worry about (but it is available if it makes the review process easier)
      2. More importantly, the rigorous test-suite provides some confidence about bugs since I'm pretty sure I cover every valid codepath. I didn't worry about totally invalid code-paths (i.e. totally invalid args) since we don't care about retaining the behavior in that case
  2. This PR leaves the original versions of the fortran routines within the core library
  • Context: we don't usually want to let this happen since that means that there could be some call-sites that still call the original fortran version, which is suboptimal for 2 reasons: (i) the logic could theoretically between the Fortran & C++ versions and (ii) it becomes very easy to make mistakes when trying to determine how to aggregates of arguments into a single struct
  • Justification for this PR: Fortran routines do indeed exist that still call the fortran versions of these routines. That is because the routines are the most pervasive routines defined in the codebase.2 I'm not very worried about drifting implementations because of our test-suite and how simple each routine is. Furthermore, I don't think we should entertain altering the arglists for these functions until we are pretty much done with transcription3.

In any case, we also have the option of simply waiting to merge this PR until most Grackle is transcribed (this is why the C++ wrapper for the fortran routines has been split off into a separate PR).


I manually confirmed that all tests definitely pass!

Footnotes

  1. In my experience, when I use my (transcription tool)[https://github.com/mabruzzo/grackle_transcribe], there aren't bugs in the original transcription from Fortran. Bugs occur when I manually transform the code (i.e. modify it look a little more like idiomatic C/C++)

  2. We could theoretically implement the C++ versions as functions with C-linkage and declare interfaces the allow the Fortran routines call the transcribed functions. This was proposed in the Transcribe contents of interpolators_g.F from Fortran to C grackle-project/grackle#160 and Modern Version Of: Transcribe contents of interpolators_g.F from Fortran to C grackle-project/grackle#251 PRs. But, doing this would require significant modification to the transcription tools. I just don't think it's worth doing.

  3. Compared to other routines, there is much less urgency to reducing the size of the arglists (since they are relatively short). But more importantly, the pervasiveness of these routines is the main reason I don't think we should refactor until we have a full picture of all the contexts where they are used. I also think that there could be relevant performance considerations (e.g. see Tabulated-mode T Optimization ideas grackle-project/grackle#212). In the short term, it could be helpful to write nice wrappers that reduce arg-lists.

brittonsmith and others added 27 commits May 21, 2025 15:57
…sting-without-editable-install

Support running subset of pygrackle test without a an editable install
…tweak

cython does not need to be a runtime-dependency
…nstruction-tweaks

Pygrackle test instruction tweaks
…Warnings

Enable Compiler Warnings in a CI job
…lding_length_fix

H2 self shielding length scale factor fix
Superficial Website Documentation Tweaks
[newchem-cpp] Minor model test refactor and adding model comparison option
mabruzzo and others added 23 commits August 13, 2025 08:55
…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`
…' into gen2024transcribe/lookup_cool_rates0d
…2024transcribe/calc_temp_cloudy_g

I needed to introduce the bugfix from PR grackle-project#367.
@mabruzzo

Copy link
Copy Markdown
Author

superseded by grackle-project#384

@mabruzzo mabruzzo closed this Aug 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants