factor out shared logic from cool1d_multi_g and calc_tdust_3d_g#21
Closed
mabruzzo wants to merge 45 commits into
Closed
factor out shared logic from cool1d_multi_g and calc_tdust_3d_g#21mabruzzo wants to merge 45 commits into
cool1d_multi_g and calc_tdust_3d_g#21mabruzzo wants to merge 45 commits into
Conversation
this parallel clause is used when converting from comoving to proper
this parallel clause is used when converting from proper to comoving
Removed unused local variables from `calc_tdust_3d_g` that followed the naming scheme used in solve_rate_cool for tracking the mass density for the mass densities of various metal species used in dust chemistry. There was logic to convert between proper and comoving units for each of these variables, but they were not actually used for anything (which is a good thing since they were never initialized). These variable declarations were probably blindly copied from somewhere else and were never removed.
Removed unused local variables from `calc_tdust_3d_g` that followed the naming scheme used in solve_rate_cool for tracking the mass density for tracking the mass densities of various metal species used in metal chemistry. There was logic to convert between proper and comoving units for each of these variables, but they were not actually used for anything (which is a good thing since they were never initialized). These variable declarations were probably blindly copied from somewhere else and were never removed.
…arts of the code.
This creates problems when we specify the answer-dir as a relative path because we commonly forget to change the directory back to the starting one. This issue will become more prominent in the next commit. - there wasn't any need to change directories in test_initialisation.py - we could remove all logic relating to explicitly changing directories by passing the ``cwd`` kwarg to ``subprocess.run`` in test_get_grackle_version.py and test_models.py (while the directory is still changed by ``subprocess.run``, they always restore the original one afterwards) More importantly, this is a good practice to have!
Previously, the test_code_examples would eagerly FAIL, if pytest was invoked to run answer-tests in "compare-mode" and the answer-file could not be found for the test_code_examples (whether or not we were planning to skip the test_code_examples). This commit relocates the FAILURE check until after the logic determining whether we should skip the test_code_examples.
… that we are building sphinx docs
Fix ReadTheDocs doc CI build
…ctor Answer test refactor
1. A bug is fixed in the Fortran Bindings 2. All references have been removed about the ``GRACKLE_DATA_DIR`` env variable. An upcoming PR (grackle-project#235) is going to overhaul this machinery, but it probably won't be merged the 3.4 release
….4Tweaks Tweaks required for 3.4 release (Fortran Binding bugfix and a few removals)
…test [newchem-cpp] Temporarily Disable OpenMP tests
Move parameter to correct place in fortran interface.
Author
|
superseded by grackle-project#277 |
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 depends on PR #20
While working on PR #20, I became aware that a bunch of logic (~200 lines) related to the calculation of dust temperatures was essentially duplicated between
cool1d_multi_gandcalc_tdust_3d_g. There were some superficial differences, but the logic very much accomplished the same thing. Thus I moved the common logic to a new Fortran subroutine calledcalc_all_tdust_gasgr_1d_g.1I think this change is hugely beneficial. If we waited to refactor this until after transcribing both routines to C/C++, there was a chance that the superficial differences in the logic may have caused us to not notice the similarities.
The logic was extracted from
cool1d_multi_g:cool1d_multi_gshould have no performance impact.calc_tdust_3d_gwill probably be a little slower (we now allocate slightly more buffers than we previously did).There is a lot of potential here to optimize the newly created
calc_all_tdust_gasgr_1d_gsubroutine. (We could probably reduce the number of allocated buffers to a number smaller than was originally used bycalc_tdust_3d_g). But that will be easier once we have transcribed the routine (and we can use structs for organization purposes). I think this fact and the benefit of reducing duplicated code definitely makes any performance penalty ofcalc_tdust_3d_ga worthwhile tradeoff.I have manually confirmed that all tests pass.2
Footnotes
I suspect that we could potentially
calc_all_tdust_gasgr_1d_gandcalc_grain_size_increment_1d, but that is a topic for another time. ↩Be advised, I was not extremely careful about wiring
calc_grain_size_increment_1dintocalc_tdust_3d_g. I kinda plopped in the new routine and addressed a few compile errors and then the tests all passed ↩