Skip to content

Conversation

@mabruzzo
Copy link
Collaborator

@mabruzzo mabruzzo commented Aug 20, 2025

This PR was previously proposed as brittonsmith#38


This should be reviewed after #383 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 #160 and Modern Version Of: Transcribe contents of interpolators_g.F from Fortran to C #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).

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 #160 and Modern Version Of: Transcribe contents of interpolators_g.F from Fortran to C #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 #212). In the short term, it could be helpful to write nice wrappers that reduce arg-lists.

@mabruzzo mabruzzo added the refactor internal reorganization or code simplification with no behavior changes label Aug 20, 2025
@mabruzzo mabruzzo moved this to Awaiting Review in New Chemistry and C++ Transcription Aug 20, 2025
@mabruzzo mabruzzo changed the base branch from newchem-cpp to main September 11, 2025 18:12
@mabruzzo mabruzzo changed the base branch from main to newchem-cpp September 11, 2025 18:13
@brittonsmith brittonsmith changed the base branch from newchem-cpp to main September 12, 2025 11:46
@brittonsmith brittonsmith changed the base branch from main to newchem-cpp September 12, 2025 11:46
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: Awaiting Review

Development

Successfully merging this pull request may close these issues.

1 participant