Transcribe of cool1d_multi_g#39
Conversation
mabruzzo
left a comment
mabruzzo
left a comment
There was a problem hiding this comment.
I meant to review this a little sooner -- sorry!
This looks fantastic! Thank you so much for doing this!
There are handful of things that need to be addressed before this can be merged into either gen2024 or gen2024-transcribe. The vast majority of these things are stuff things that I didn't tell you about ahead of time (and they have to do with some basic conventions I have started following, or lessons that I've learned while transcribing)
I left a handful of comments requesting changes (a bunch of them are things that I didn't really document, but are conventions that I have been following).
There are 3 other larger-scale changes that would be useful:
-
could you please remove
cool1d_multi_g.Ffrom the repository? -
Would you mind creating a commit that remove as many references relating to addition/subtraction an offset of 1 and the
jandkindices?- I think we actually talked about doing this. I was probably much too vague
- For context: the
jandkvariables are tracked byidx_range. As you may recall, the struct membersidx_range.jp1andidx_range.kp1are abbreviations for "j plus 1" and "k plus 1". This was all done to reduce zero-indexed variables (and we don't think about adding/subtracting one) - this can base that just consists of performing search-and-replace that performs the substitutions
idx_range.jp1-1->idx_range.jandidx_range.kp1-1->idx_range.k. - It will be a lot easier to do now, than if we wait until later since the formatting of the strings of text that need to be replaced are currently very consistent (they were auto-generated).
- If transcription worked properly, there should no longer be any references to
idx_range.kp1oridx_range.jp1(if any references persist, that may be a sign that something went wrong during transcription. Alternatively this could just happen in places where you modified the code)
-
Remove all occurrences of
i-1from fromcool1d_multi_g-cpp.C. This is going to involve modifying the for-loops so thatiis a 0-based index.- this can all be accomplished via search-and-replace operations.
- In case you find it useful, I wrote a simple command-line tool to help do this (it requires a little manual intervention). You can find it here. I think the
--helpcommand does a reasonable job explaining how to use the tool (but, if you're interested I can provide more elaboration)
-
After you merge in the latest changes from
gen2024-transcribe, it would be great if you could replacing invocations of Fortran routines with wrapper functions. There are some minor differences in the wrapper function's interface (to reflect the planned C++ function's interface), but most should be fairly to straightforward to understand. The easiest functions to replace are:grackle::impl::fortran_wrapper::calc_temp1d_cloudy_geasily replacesFORTRAN_NAME(calc_temp1d_cloudy_g)grackle::impl::fortran_wrapper::interpolate_2d_geasily replacesFORTRAN_NAME(interpolate_2d_g)grackle::impl::fortran_wrapper::interpolate_3d_geasily replaces each occurrence ofFORTRAN_NAME(interpolate_3d_g)- as an aside: whenever I have a function that calls multiple wrapper functions, I commonly add namespace
f_wrap = ::grackle::impl::fortran_wrapper;near the start of the function (i.e. the wrapper-namespace is shortened tof_wrapwithin the function's scope). But obviously, this isn't necessary (you should do whatever you find most useful)
I left comments that describe the order in which you should replace
FORTRAN_NAME(calc_grain_size_increment_1d)andFORTRAN_NAME(calc_all_tdust_gasgr_1d_g).1 I think that this should cover all fortran functions with C++ wrappers.2
Again, this looks great! I really appreciate the help! This goes without saying, but If you disagree with any of my suggestions, you should obviously feel free to push back!
Footnotes
| grackle::impl::CoolHeatScratchBuf coolingheating_buf | ||
| ); | ||
|
|
||
| #endif /* MY_FILE_CPP_H */ |
There was a problem hiding this comment.
| #endif /* MY_FILE_CPP_H */ | |
| #endif /* COOL_1D_MULTI_G_HPP */ |
| @@ -23,7 +23,7 @@ OBJS_CONFIG_LIB = \ | |||
| cool1d_cloudy_g.lo \ | |||
| cool1d_cloudy_old_tables_g.lo \ | |||
| cool1d_multi_g.lo \ | |||
There was a problem hiding this comment.
This change is required to avoid breaking the classic build-system
| cool1d_multi_g.lo \ |
| #include "fortran_func_decls.h" | ||
| #include "utils-cpp.hpp" | ||
|
|
||
| void cool1d_multi_g( |
There was a problem hiding this comment.
Related to the earlier namespace suggestion I made:
| void cool1d_multi_g( | |
| void grackle::impl::cool1d_multi_g( |
| // Add dust opacity. | ||
| // if (idspecies .eq. 0), dust opacity is overestimated at Td > 50 K | ||
| // We better not include dust opacity. |
There was a problem hiding this comment.
| // Add dust opacity. | |
| // if (idspecies .eq. 0), dust opacity is overestimated at Td > 50 K | |
| // We better not include dust opacity. | |
| // Add contributions from dust opacity to alpha, the linear absorption coefficient | |
| // | |
| // The original Fortran version of this function had the following 2 comments: | |
| // ! if (idspecies .eq. 0), dust opacity is overestimated at Td > 50 K | |
| // ! We better not include dust opacity. | |
| // It's a little unclear how relevant these comments actually are. |
| dom = internalu.urho*(std::pow(internalu.a_value,3))/mh_local_var; | ||
| dom_inv = 1./dom; | ||
| tbase1 = internalu.tbase1; | ||
| xbase1 = internalu.uxyz/(internalu.a_value*internalu.a_units); // uxyz is [x]*a = [x]*[a]*a' ' | ||
| dbase1 = internalu.urho*std::pow((internalu.a_value*internalu.a_units),3); // urho is [dens]/a^3 = [dens]/([a]*a')^3 ' | ||
| coolunit = (std::pow(internalu.a_units,5) * std::pow(xbase1,2) * std::pow(mh_local_var,2)) / (std::pow(tbase1,3) * dbase1); |
There was a problem hiding this comment.
This particular logic had been duplicated between 4 or 5 different Fortran files. I designed the internalu related functionality to factor out this exact functionality (I took care to preserve the order of operations to prevent drift in answer-test results). I think it is probably worth doing this before the logic is able to start diverging.
The following suggestion shows how to integrate the internalu logic (if you want to do this). If you choose to make this change, you should remove the declaration of xbase1 and dbase1 variables, and explicitly confirm that this doesn't break any tests.
| dom = internalu.urho*(std::pow(internalu.a_value,3))/mh_local_var; | |
| dom_inv = 1./dom; | |
| tbase1 = internalu.tbase1; | |
| xbase1 = internalu.uxyz/(internalu.a_value*internalu.a_units); // uxyz is [x]*a = [x]*[a]*a' ' | |
| dbase1 = internalu.urho*std::pow((internalu.a_value*internalu.a_units),3); // urho is [dens]/a^3 = [dens]/([a]*a')^3 ' | |
| coolunit = (std::pow(internalu.a_units,5) * std::pow(xbase1,2) * std::pow(mh_local_var,2)) / (std::pow(tbase1,3) * dbase1); | |
| dom = internalu_calc_dom_(internalu); | |
| dom_inv = 1./dom; | |
| tbase1 = internalu.tbase1; | |
| coolunit = internalu.coolunit; |
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
Superficial Website Documentation Tweaks
[newchem-cpp] Minor model test refactor and adding model comparison option
…eaks fixing up config.yml
…eaks address the remaining compiler warnings
Fix spacing issues in docs.
Relocated all CMake-dependency-information into dependencies.cmake
…phson-cleanup-1of2
…phson-cleanup-1of2
…' into gen2024transcribe/solve_rate_cool-refactor
…' into gen2024transcribe/lookup_cool_rates0d
…scribe/lookup_cool_rates0d_cleanup
…2024transcribe/calc_temp_cloudy_g I needed to introduce the bugfix from PR grackle-project#367.
…cribe/scale_fields_table
…cribe/interpolate-wrappers
…nscribe/calc_tdust_3d
…/scale_fields_dust
Background ---------- Previously we had replaced 99% uses of the Flake8 linter with the Ruff linter (not to be confused with the Ruff formatter). We had continued to use the Flake8 linter to deal with the following 2 files: 1. **gracklepy/utilities/testing.py** 2. **gracklepy/__init__.py** In more detail, we were suppressing certain warnings in these 2 files from Flake8, and transistioning to using the Ruff formatter for these files would involve making a few changes. More recently PRs grackle-project#368 and grackle-project#369 introduced the required changes into the main branch. Now that those changes have been ported to the newchem-cpp branch, we are able to make this change. What Changed ------------ I essentially changed 3 things: 1. We no longer install and use flake8 in the continuous integration 2. I removed the **setup.cfg** file (it was **only** being used to track flake8 configuration information 3. I needed to tweak **pyproject.toml** to both: - stop excluding the **gracklepy/utilities/testing.py** and **gracklepy/__init__.py** files from the linter - stop listing flake8 as a development dependency
…e/step_rate_newton_raphson-cleanup-1of2 [newchem-cpp] `step_rate_newton_raphson` cleanup (1/2)
…e/solve_rate_cool-refactor [newchem-cpp] light refactoring of `solve_rate_cool`
[newchem-cpp] Remove last references to `Flake8`
…_dust' into gen2024_transcribe_cool1d_multi_g
…024_transcribe_cool1d_multi_g
|
Moved to main grackle repo as PR to the newchem_cpp branch: |
4 participants
This branch has been created from @mabruzzo gen2024-transcribe branch (https://github.com/mabruzzo/grackle/tree/gen2024-transcribe)